Re: [RFC PATCH v3 6/9] NFSv4/flexfiles: Commit path updates for striped layouts

[Nathan Chancellor <nathan@kernel.org>]

Hi Jonathan,

On Thu, Sep 18, 2025 at 01:33:07PM +0000, Jonathan Curley wrote:
...
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> index 79700c18762c..b061e7a576cf 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> @@ -605,6 +605,26 @@ ff_layout_free_lseg(struct pnfs_layout_segment *lseg)
>  	_ff_layout_free_lseg(fls);
>  }
>  
> +static u32 calc_mirror_idx_from_commit(struct pnfs_layout_segment *lseg,
> +				       u32 commit_index)
> +{
> +	struct nfs4_ff_layout_segment *flseg = FF_LAYOUT_LSEG(lseg);
> +	u64 mirror_idx = commit_index;
> +
> +	do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
> +
> +	return mirror_idx;
> +}
> +
> +static u32 calc_dss_id_from_commit(struct pnfs_layout_segment *lseg,
> +				   u32 commit_index)
> +{
> +	struct nfs4_ff_layout_segment *flseg = FF_LAYOUT_LSEG(lseg);
> +	u64 mirror_idx = commit_index;
> +
> +	return do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
> +}
> +

This change is in -next as commit 67ee714244df ("NFSv4/flexfiles: Commit
path updates for striped layouts"), where it causes warnings for 32-bit
platforms:

  In file included from arch/arm/include/asm/div64.h:114,
                   from include/linux/math.h:6,
                   from include/linux/kernel.h:27,
                   from include/linux/uio.h:8,
                   from include/linux/socket.h:8,
                   from include/uapi/linux/in.h:25,
                   from include/linux/in.h:19,
                   from include/linux/nfs_fs.h:22,
                   from fs/nfs/flexfilelayout/flexfilelayout.c:10:
  fs/nfs/flexfilelayout/flexfilelayout.c: In function 'calc_mirror_idx_from_commit':
  include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
    183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
        |                                   ^~
  fs/nfs/flexfilelayout/flexfilelayout.c:685:9: note: in expansion of macro 'do_div'
    685 |         do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |         ^~~~~~
  In file included from include/linux/array_size.h:5,
                   from include/linux/kernel.h:16:
  include/asm-generic/div64.h:195:32: error: right shift count >= width of type [-Werror=shift-count-overflow]
    195 |         } else if (likely(((n) >> 32) == 0)) {          \
        |                                ^~
  include/linux/compiler.h:76:45: note: in definition of macro 'likely'
     76 | # define likely(x)      __builtin_expect(!!(x), 1)
        |                                             ^
  fs/nfs/flexfilelayout/flexfilelayout.c:685:9: note: in expansion of macro 'do_div'
    685 |         do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |         ^~~~~~
  include/asm-generic/div64.h:199:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Wincompatible-pointer-types]
    199 |                 __rem = __div64_32(&(n), __base);       \
        |                                    ^~~~
        |                                    |
        |                                    u32 * {aka unsigned int *}
  fs/nfs/flexfilelayout/flexfilelayout.c:685:9: note: in expansion of macro 'do_div'
    685 |         do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |         ^~~~~~
  arch/arm/include/asm/div64.h:24:45: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
     24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
        |                                   ~~~~~~~~~~^
  fs/nfs/flexfilelayout/flexfilelayout.c: In function 'calc_dss_id_from_commit':
  include/asm-generic/div64.h:183:35: error: comparison of distinct pointer types lacks a cast [-Werror=compare-distinct-pointer-types]
    183 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
        |                                   ^~
  fs/nfs/flexfilelayout/flexfilelayout.c:696:16: note: in expansion of macro 'do_div'
    696 |         return do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |                ^~~~~~
  include/asm-generic/div64.h:195:32: error: right shift count >= width of type [-Werror=shift-count-overflow]
    195 |         } else if (likely(((n) >> 32) == 0)) {          \
        |                                ^~
  include/linux/compiler.h:76:45: note: in definition of macro 'likely'
     76 | # define likely(x)      __builtin_expect(!!(x), 1)
        |                                             ^
  fs/nfs/flexfilelayout/flexfilelayout.c:696:16: note: in expansion of macro 'do_div'
    696 |         return do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |                ^~~~~~
  include/asm-generic/div64.h:199:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Wincompatible-pointer-types]
    199 |                 __rem = __div64_32(&(n), __base);       \
        |                                    ^~~~
        |                                    |
        |                                    u32 * {aka unsigned int *}
  fs/nfs/flexfilelayout/flexfilelayout.c:696:16: note: in expansion of macro 'do_div'
    696 |         return do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
        |                ^~~~~~
  arch/arm/include/asm/div64.h:24:45: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'u32 *' {aka 'unsigned int *'}
     24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
        |                                   ~~~~~~~~~~^
  cc1: all warnings being treated as errors

do_div() should only be called with a 64-bit dividend. I don't see why
this needs a division helper because mirror_idx and ->dss_count are both
u32?

It seems like the following diff would be sufficient to resolve this if
I am understanding correctly but I am not familiar with this code. Could
even eliminate *flseg as well, it would fit within 80 characters.

Cheers,
Nathan

diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
index 0eca9367b610..c726309bfbe1 100644
--- a/fs/nfs/flexfilelayout/flexfilelayout.c
+++ b/fs/nfs/flexfilelayout/flexfilelayout.c
@@ -680,20 +680,16 @@ static u32 calc_mirror_idx_from_commit(struct pnfs_layout_segment *lseg,
 				       u32 commit_index)
 {
 	struct nfs4_ff_layout_segment *flseg = FF_LAYOUT_LSEG(lseg);
-	u32 mirror_idx = commit_index;
 
-	do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
-
-	return mirror_idx;
+	return commit_index / flseg->mirror_array[0]->dss_count;
 }
 
 static u32 calc_dss_id_from_commit(struct pnfs_layout_segment *lseg,
 				   u32 commit_index)
 {
 	struct nfs4_ff_layout_segment *flseg = FF_LAYOUT_LSEG(lseg);
-	u32 mirror_idx = commit_index;
 
-	return do_div(mirror_idx, flseg->mirror_array[0]->dss_count);
+	return commit_index % flseg->mirror_array[0]->dss_count;
 }
 
 static void