Re: [PATCH v3 03/13] nfsd: drop the ncf_cb_bmap field

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2024-09-04 at 11:20 -0400, Chuck Lever wrote:
> On Thu, Aug 29, 2024 at 09:26:41AM -0400, Jeff Layton wrote:
> > This is always the same value, and in a later patch we're going to need
> > to set bits in WORD2. We can simplify this code and save a little space
> > in the delegation too. Just hardcode the bitmap in the callback encode
> > function.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> 
> OK, lurching forward on this ;-)
> 
>  - The first patch in v3 was applied to v6.11-rc.
>  - The second patch is now in nfsd-next.
>  - I've squashed the sixth and eighth patches into nfsd-next.
> 
> I'm replying to this patch because this one seems like a step
> backwards to me: the bitmap values are implementation-dependent,
> and this code will eventually be automatically generated based only
> on the protocol, not the local implementation. IMO, architecturally,
> bitmap values should be set at the proc layer, not in the encoders.
> 
> I've gone back and forth on whether to just go with it for now, but
> I thought I'd mention it here to see if this change is truly
> necessary for what follows. If it can't be replaced, I will suck it
> up and fix it up later when this encoder is converted to an xdrgen-
> manufactured one.

It's not truly necessary, but I don't see why it's important that we
keep a record of the full-blown bitmap here. The ncf_cb_bmap field is a
field in the delegation record, and it has to be carried around in
perpetuity. This value is always set by the server and there are only a
few legit bit combinations that can be set in it.

We certainly can keep this bitmap around, but as I said in the
description, the delstid draft grows this bitmap to 3 words, and if we
want to be a purists about storing a bitmap, then that will also
require us to keep the bitmap size (in another 32-bit word), since we
don't always want to set anything in the third word. That's already 24
extra bits per delegation, and we'll be adding new fields for the
timestamps in a later patch.

I don't see the benefit here.

> I've tried to apply a few of the following patches in this series,
> but by 9/13, things stop compiling. So I will let you rebase your
> work on the current nfsd-next and redrive it, and we can go from
> there.
> 

Great. I'll rebase and re-test.

PS: I'm working on a couple of pynfs tests for CB_GETATTR as well. I'll
post them once I have them working. I've also found nfsd bugs in how we
currently proxy the attrs. I'll have patches for that in the next
posting (once my question on the IETF list is answered).

> 
> > ---
> >  fs/nfsd/nfs4callback.c | 5 ++++-
> >  fs/nfsd/nfs4state.c    | 1 -
> >  fs/nfsd/state.h        | 1 -
> >  3 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index b5b3ab9d719a..0c49e31d4350 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -364,10 +364,13 @@ encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
> >  	struct nfs4_delegation *dp =
> >  		container_of(fattr, struct nfs4_delegation, dl_cb_fattr);
> >  	struct knfsd_fh *fh = &dp->dl_stid.sc_file->fi_fhandle;
> > +	u32 bmap[1];
> > +
> > +	bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> >  
> >  	encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
> >  	encode_nfs_fh4(xdr, fh);
> > -	encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> > +	encode_bitmap4(xdr, bmap, ARRAY_SIZE(bmap));
> >  	hdr->nops++;
> >  }
> >  
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 8835930ecee6..6844ae9ea350 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1183,7 +1183,6 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> >  	nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
> >  			&nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
> >  	dp->dl_cb_fattr.ncf_file_modified = false;
> > -	dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> >  	get_nfs4_file(fp);
> >  	dp->dl_stid.sc_file = fp;
> >  	return dp;
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 79c743c01a47..ac3a29224806 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -138,7 +138,6 @@ struct nfs4_cpntf_state {
> >  struct nfs4_cb_fattr {
> >  	struct nfsd4_callback ncf_getattr;
> >  	u32 ncf_cb_status;
> > -	u32 ncf_cb_bmap[1];
> >  
> >  	/* from CB_GETATTR reply */
> >  	u64 ncf_cb_change;
> > 
> > -- 
> > 2.46.0
> > 
> 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>