Re: [PATCH 1/3] NFSv4/flexfiles: report the intended opnum when DS connection setup fails

Hi Ben,

Our logs are full of those  OP_ILLEGAL errors. Thanks for making them more useful.
The change makes sense and looks valid.

Reviewed-by: Tigran Mkrtchyan <[email protected]>

— Tigran

> On 13. Aug 2026, at 20:07, Benjamin Coddington <[email protected]> wrote:
> 
> When nfs4_ff_layout_prepare_ds() cannot establish a connection to a
> data server, it records a device error with opnum OP_ILLEGAL, since no
> operation ever made it to the wire.  But servers can use the ff_ioerr4
> opnum to distinguish failed read-class operations from write-class
> operations when deciding how to recover the affected mirror -- a mirror
> that may have missed writes needs to be brought back in sync, while one
> that merely failed to serve a read does not.  OP_ILLEGAL gives the
> server nothing to act on.
> 
> Every caller of nfs4_ff_layout_prepare_ds() knows which operation it
> was preparing to send, and the existing fail_return argument already
> divides the callers along the same boundary (false for READ, true for
> WRITE and COMMIT).  Replace the boolean with the intended opnum, derive
> the layout return decision from it, and report it in the tracked device
> error instead of OP_ILLEGAL.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Benjamin Coddington <[email protected]>
> ---
> fs/nfs/flexfilelayout/flexfilelayout.c    | 10 +++++-----
> fs/nfs/flexfilelayout/flexfilelayout.h    |  2 +-
> fs/nfs/flexfilelayout/flexfilelayoutdev.c | 14 ++++++++------
> 3 files changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.c b/fs/nfs/flexfilelayout/flexfilelayout.c
> index 7fe8b91fa47c..2c30e62881a1 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.c
> @@ -893,7 +893,7 @@ ff_layout_choose_ds_for_read(struct pnfs_layout_segment *lseg,
>            fls->stripe_unit,
>            fls->mirror_array[idx]->dss_count,
>            offset);
> -        ds = nfs4_ff_layout_prepare_ds(lseg, mirror, *dss_id, false);
> +        ds = nfs4_ff_layout_prepare_ds(lseg, mirror, *dss_id, OP_READ);
>        if (IS_ERR(ds))
>            continue;
> 
> @@ -1135,7 +1135,7 @@ ff_layout_pg_init_write(struct nfs_pageio_descriptor *pgio,
>            mirror->dss_count,
>            req_offset(req));
>        ds = nfs4_ff_layout_prepare_ds(pgio->pg_lseg, mirror,
> -                           dss_id, true);
> +                           dss_id, OP_WRITE);
>        if (IS_ERR(ds)) {
>            if (!ff_layout_no_fallback_to_mds(pgio->pg_lseg))
>                goto out_mds;
> @@ -2184,7 +2184,7 @@ ff_layout_read_pagelist(struct nfs_pgio_header *hdr)
>        FF_LAYOUT_LSEG(lseg)->stripe_unit,
>        mirror->dss_count,
>        offset);
> -    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, false);
> +    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, OP_READ);
>    if (IS_ERR(ds)) {
>        ds_fatal_error = nfs_error_is_fatal(PTR_ERR(ds));
>        goto out_failed;
> @@ -2275,7 +2275,7 @@ ff_layout_write_pagelist(struct nfs_pgio_header *hdr, int sync)
>        FF_LAYOUT_LSEG(lseg)->stripe_unit,
>        mirror->dss_count,
>        offset);
> -    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, true);
> +    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, OP_WRITE);
>    if (IS_ERR(ds)) {
>        ds_fatal_error = nfs_error_is_fatal(PTR_ERR(ds));
>        goto out_failed;
> @@ -2376,7 +2376,7 @@ static int ff_layout_initiate_commit(struct nfs_commit_data *data, int how)
>    idx = calc_mirror_idx_from_commit(lseg, data->ds_commit_index);
>    mirror = FF_LAYOUT_COMP(lseg, idx);
>    dss_id = calc_dss_id_from_commit(lseg, data->ds_commit_index);
> -    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, true);
> +    ds = nfs4_ff_layout_prepare_ds(lseg, mirror, dss_id, OP_COMMIT);
>    if (IS_ERR(ds))
>        goto out_err;
> 
> diff --git a/fs/nfs/flexfilelayout/flexfilelayout.h b/fs/nfs/flexfilelayout/flexfilelayout.h
> index a5bd00f69e82..8e3a1e45ea68 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayout.h
> +++ b/fs/nfs/flexfilelayout/flexfilelayout.h
> @@ -252,7 +252,7 @@ struct nfs4_pnfs_ds *
> nfs4_ff_layout_prepare_ds(struct pnfs_layout_segment *lseg,
>              struct nfs4_ff_layout_mirror *mirror,
>              u32 dss_id,
> -              bool fail_return);
> +              enum nfs_opnum4 opnum);
> 
> struct rpc_clnt *
> nfs4_ff_find_or_create_ds_client(struct nfs4_ff_layout_mirror *mirror,
> diff --git a/fs/nfs/flexfilelayout/flexfilelayoutdev.c b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> index 8be5c730e101..5920ba529a0b 100644
> --- a/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> +++ b/fs/nfs/flexfilelayout/flexfilelayoutdev.c
> @@ -359,7 +359,7 @@ ff_layout_init_mirror_ds(struct pnfs_layout_hdr *lo,
>  * @lseg: the layout segment we're operating on
>  * @mirror: layout mirror describing the DS to use
>  * @dss_id: DS stripe id to select stripe to use
> - * @fail_return: return layout on connect failure?
> + * @opnum: operation this connection is being prepared for
>  *
>  * Try to prepare a DS connection to accept an RPC call. This involves
>  * selecting a mirror to use and connecting the client to it if it's not
> @@ -367,8 +367,10 @@ ff_layout_init_mirror_ds(struct pnfs_layout_hdr *lo,
>  *
>  * Since we only need a single functioning mirror to satisfy a read, we don't
>  * want to return the layout if there is one. For writes though, any down
> - * mirror should result in a LAYOUTRETURN. @fail_return is how we distinguish
> - * between the two cases.
> + * mirror should result in a LAYOUTRETURN. @opnum is how we distinguish
> + * between the two cases. On failure, @opnum is also reported in the tracked
> + * device error so that the server can tell which class of I/O the client
> + * was unable to send to the mirror.
>  *
>  * Returns a pointer to a connected DS object on success or NULL on failure.
>  */
> @@ -376,7 +378,7 @@ struct nfs4_pnfs_ds *
> nfs4_ff_layout_prepare_ds(struct pnfs_layout_segment *lseg,
>              struct nfs4_ff_layout_mirror *mirror,
>              u32 dss_id,
> -              bool fail_return)
> +              enum nfs_opnum4 opnum)
> {
>    struct nfs4_pnfs_ds *ds;
>    struct inode *ino = lseg->pls_layout->plh_inode;
> @@ -423,9 +425,9 @@ nfs4_ff_layout_prepare_ds(struct pnfs_layout_segment *lseg,
>    ff_layout_track_ds_error(FF_LAYOUT_FROM_HDR(lseg->pls_layout),
>                 mirror, dss_id, lseg->pls_range.offset,
>                 lseg->pls_range.length, NFS4ERR_NXIO,
> -                 OP_ILLEGAL, GFP_NOIO);
> +                 opnum, GFP_NOIO);
>    ff_layout_send_layouterror(lseg);
> -    if (fail_return || !ff_layout_has_available_ds(lseg))
> +    if (opnum != OP_READ || !ff_layout_has_available_ds(lseg))
>        pnfs_error_mark_layout_for_return(ino, lseg);
>    ds = ERR_PTR(status);
> out:
> --
> 2.53.0
> 
>