[PATCH 1/2] fix failed compilation due to lookup_in v5.11

[Su Yue]

Linux commit 4e7b5671c6a8 ("block: remove i_bdev") changed function
lookup_bdev():

-struct block_device *lookup_bdev(const char *pathname)
+int lookup_bdev(const char *pathname, dev_t *dev)

And APFS module commit 3a2e3369d251 ("Take exclusive ownership of block
device on mount") didn't notice the change. It causes failed compilation
in v5.11 and later:

=======================================================================
/root/rw/super.c:1128:29: error: too few arguments to function call,
expected 2, have 1
bdev = lookup_bdev(dev_name);
~~~~~~~~~~~ ^
./include/linux/blkdev.h:1984:5: note: 'lookup_bdev' declared here
int lookup_bdev(const char *pathname, dev_t *dev);
^
1 error generated.
=======================================================================

Introduce new version apfs_nx_find_by_dev() for v5.11 and let
apfs_attach_nxi() call new version lookup_bdev(). Both two changes are
inside version preprocessors.

Signed-off-by: Su Yue <l...@damenly.su>
---
super.c | 37 +++++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)

diff --git a/super.c b/super.c
index a65e6ea801dc..5af2ea0bdc06 100644
--- a/super.c
+++ b/super.c
@@ -23,6 +23,7 @@
DEFINE_MUTEX(nxs_mutex);
static LIST_HEAD(nxs);

+#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 11, 0)
/**
* apfs_nx_find_by_dev - Search for a device in the list of mounted containers
* @bdev: block device for the wanted container
@@ -44,6 +45,29 @@ static struct apfs_nxsb_info *apfs_nx_find_by_dev(struct block_device *bdev)
return NULL;
}

+#else
+/**
+ * apfs_nx_find_by_dev - Search for a device in the list of mounted containers
+ * @dev: device number of block device for the wanted container
+ *
+ * Returns a pointer to the container structure in the list, or NULL if the
+ * container isn't currently mounted.
+ */
+static struct apfs_nxsb_info *apfs_nx_find_by_dev(dev_t dev)
+{
+ struct apfs_nxsb_info *curr;
+
+ lockdep_assert_held(&nxs_mutex);
+ list_for_each_entry(curr, &nxs, nx_list) {
+ struct block_device *curr_bdev = curr->nx_bdev;
+
+ if (curr_bdev->bd_dev != dev)
+ return curr;
+ }
+ return NULL;
+}
+#endif
+
/**
* apfs_sb_set_blocksize - Set the block size for the container's device
* @sb: superblock structure
@@ -1122,6 +1146,7 @@ static int apfs_attach_nxi(struct apfs_sb_info *sbi, const char *dev_name, fmode
{
struct apfs_nxsb_info *nxi;
struct block_device *bdev;
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 11, 0)

lockdep_assert_held(&nxs_mutex);

@@ -1132,6 +1157,18 @@ static int apfs_attach_nxi(struct apfs_sb_info *sbi, const char *dev_name, fmode
nxi = apfs_nx_find_by_dev(bdev);
bdput(bdev);
bdev = NULL;
+#else
+ dev_t dev;
+ int ret;
+
+ lockdep_assert_held(&nxs_mutex);
+
+ ret = lookup_bdev(dev_name, &dev);
+ if (ret)
+ return ret;
+ nxi = apfs_nx_find_by_dev(dev);
+#endif
+
if (!nxi) {
nxi = kzalloc(sizeof(*nxi), GFP_KERNEL);
if (!nxi)
--
2.30.1

[Su Yue]

Since 0xFFFFFFFF is not power of 2 abviously, one call of do_div() does
one division and one multiplication.
And because only reminder is neededm doing modulus is more explicit.

Signed-off-by: Su Yue <l...@damenly.su>
---

object.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/object.c b/object.c
index ce137cd53a09..8da8d7ca2fd3 100644
--- a/object.c
+++ b/object.c
@@ -27,9 +27,9 @@ static u64 apfs_fletcher64(void *addr, size_t len)
}

c1 = sum1 + sum2;
- c1 = 0xFFFFFFFF - do_div(c1, 0xFFFFFFFF);
+ c1 = 0xFFFFFFFF - c1 % 0xFFFFFFFF;
c2 = sum1 + c1;
- c2 = 0xFFFFFFFF - do_div(c2, 0xFFFFFFFF);
+ c2 = 0xFFFFFFFF - c2 % 0xFFFFFFFF;

return (c2 << 32) | c1;
}
--
2.30.1

[Su Yue]

[Ernesto A. Fernández]

Hi,

On Wed, Jun 23, 2021 at 01:34:21PM +0800, Su Yue wrote:
> Linux commit 4e7b5671c6a8 ("block: remove i_bdev") changed function
> lookup_bdev():
>
> -struct block_device *lookup_bdev(const char *pathname)
> +int lookup_bdev(const char *pathname, dev_t *dev)
>
> And APFS module commit 3a2e3369d251 ("Take exclusive ownership of block
> device on mount") didn't notice the change. It causes failed compilation
> in v5.11 and later:
>
> =======================================================================
> /root/rw/super.c:1128:29: error: too few arguments to function call,
> expected 2, have 1
> bdev = lookup_bdev(dev_name);
> ~~~~~~~~~~~ ^
> ./include/linux/blkdev.h:1984:5: note: 'lookup_bdev' declared here
> int lookup_bdev(const char *pathname, dev_t *dev);
> ^
> 1 error generated.
> =======================================================================
>
> Introduce new version apfs_nx_find_by_dev() for v5.11 and let
> apfs_attach_nxi() call new version lookup_bdev(). Both two changes are
> inside version preprocessors.
>
> Signed-off-by: Su Yue <l...@damenly.su>

Thanks for the patch. I think you are in the right direction, but you have
a bug here:

^^
You are returning the first block device in the list that *doesn't* match.

I think it's better to avoid the preprocessor here, and just use your new
apfs_nx_find_by_dev() for all kernel versions. That way I can test most of
the code myself in 5.3. No need to resend, I'll push the change myself, but
let me know when you can if the compilation is fixed.

Ernesto

> --
> You received this message because you are subscribed to the Google Groups "linux-apfs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-apfs+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-apfs/20210623053422.1347-1-l%40damenly.su.

[Ernesto A. Fernández]

Hi,

On Wed, Jun 23, 2021 at 01:37:54PM +0800, Su Yue wrote:
> Since 0xFFFFFFFF is not power of 2 abviously, one call of do_div() does
> one division and one multiplication.
> And because only reminder is neededm doing modulus is more explicit.

The do_div() macros are a little awkward, but they are needed to support
32-bit architectures. If this was userland, the compiler would silently link
to a runtime library, but the kernel is freestanding code.

Do you see a difference in performance from this change?

Ernesto

> Signed-off-by: Su Yue <l...@damenly.su>
> ---
> object.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/object.c b/object.c
> index ce137cd53a09..8da8d7ca2fd3 100644
> --- a/object.c
> +++ b/object.c
> @@ -27,9 +27,9 @@ static u64 apfs_fletcher64(void *addr, size_t len)
> }
>
> c1 = sum1 + sum2;
> - c1 = 0xFFFFFFFF - do_div(c1, 0xFFFFFFFF);
> + c1 = 0xFFFFFFFF - c1 % 0xFFFFFFFF;
> c2 = sum1 + c1;
> - c2 = 0xFFFFFFFF - do_div(c2, 0xFFFFFFFF);
> + c2 = 0xFFFFFFFF - c2 % 0xFFFFFFFF;
>
> return (c2 << 32) | c1;
> }
> --
> 2.30.1
>

> --
> You received this message because you are subscribed to the Google Groups "linux-apfs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-apfs+...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/linux-apfs/20210623053754.1612-1-l%40damenly.su.

[Su Yue]

On Thu 24 Jun 2021 at 02:41, Ernesto A. Fernández

<ernesto.mn...@gmail.com> wrote:

> Hi,
>
> On Wed, Jun 23, 2021 at 01:37:54PM +0800, Su Yue wrote:
>> Since 0xFFFFFFFF is not power of 2 abviously, one call of
>> do_div() does
>> one division and one multiplication.
>> And because only reminder is neededm doing modulus is more
>> explicit.
>
> The do_div() macros are a little awkward, but they are needed to
> support
> 32-bit architectures. If this was userland, the compiler would
> silently link
> to a runtime library, but the kernel is freestanding code.
>
> Do you see a difference in performance from this change?
>

No. The "80-20" Rule should be applied here. The change shouldn't
improve
much performance impact.

While copying the csum function to my userspace project, the
do_div()
took my some time to figure out only the reminder is need. And
then I
just post the patch.
I do not have so strong willing for this change, so it up to you
;)

--
Su

[Su Yue]

On Thu 24 Jun 2021 at 01:25, Ernesto A. Fernández

Yes, a stupid mistake. :facepalm:

> I think it's better to avoid the preprocessor here, and just use
> your new
> apfs_nx_find_by_dev() for all kernel versions. That way I can
> test most of
> the code myself in 5.3. No need to resend, I'll push the change
> myself, but
> let me know when you can if the compilation is fixed.
>

Thanks for your patience. I checked the code and the compilation
with
latest kernel succeeded.

--
Su