diff mbox series

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

Message ID 20240829-delstid-v3-3-271c60806c5d@kernel.org (mailing list archive)
State New
Headers show
Series nfsd: implement the "delstid" draft | expand

Commit Message

Jeff Layton Aug. 29, 2024, 1:26 p.m. UTC
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>
---
 fs/nfsd/nfs4callback.c | 5 ++++-
 fs/nfsd/nfs4state.c    | 1 -
 fs/nfsd/state.h        | 1 -
 3 files changed, 4 insertions(+), 3 deletions(-)

Comments

Chuck Lever Sept. 4, 2024, 3:20 p.m. UTC | #1
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.

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.


> ---
>  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 Sept. 4, 2024, 4:58 p.m. UTC | #2
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
> > 
> 
> 
>
Chuck Lever Sept. 4, 2024, 5:28 p.m. UTC | #3
> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> 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,

Fwiw, it isn't purism about storing the bitmap, it's
purism about adopting machine-generated XDR marshaling/
unmarshaling code. The patch adds non-marshaling logic
in the encoder. Either we'll have to add a way to handle
that in xdrgen eventually, or we'll have to exclude this
encoder from machine generation. (This is a ways down the
road, of course)


> 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.

Understood, there's a memory scalability issue.

There are other ways to go about this that do not grow
the size of the delegation data structure, I think. For
instance, you could store the handful of actual valid
bitmap values in read-only memory, and have the proc
function select and reference one of them. IIRC the
client already does this for certain GETATTR operations.

So, leave this patch as is, and I will deal with it
later when we convert the callback XDR functions.


--
Chuck Lever
Jeff Layton Sept. 4, 2024, 5:39 p.m. UTC | #4
On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> 
> > On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > 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,
> 
> Fwiw, it isn't purism about storing the bitmap, it's
> purism about adopting machine-generated XDR marshaling/
> unmarshaling code. The patch adds non-marshaling logic
> in the encoder. Either we'll have to add a way to handle
> that in xdrgen eventually, or we'll have to exclude this
> encoder from machine generation. (This is a ways down the
> road, of course)
>

Understood. I'll note that this function works with a nfs4_delegation
pointer too, which is not part of the protocol spec.

Once we move to autogenerated code, we will always have a hand-rolled
"glue" layer that morphs our internal representation of these sorts of
objects into a form that the xdrgen code requires. Storing this info as
a flag in the delegation makes more sense to me, as the glue layer
should massage that into the needed form.

> 
> > 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.
> 
> Understood, there's a memory scalability issue.
> 
> There are other ways to go about this that do not grow
> the size of the delegation data structure, I think. For
> instance, you could store the handful of actual valid
> bitmap values in read-only memory, and have the proc
> function select and reference one of them. IIRC the
> client already does this for certain GETATTR operations.
> 

Unfortunately, it turns out that the bitmap is a bit more complicated,
and we don't quite do it right today. I did a more careful read of
section 10.4.3 in RFC8881. It has this pseudocode:

    if (!modified) {
        do CB_GETATTR for change and size;

        if (cc != sc)
            modified = TRUE;
    } else {
        do CB_GETATTR for size;
    }

    if (modified) {
        sc = sc + 1;
        time_modify = time_metadata = current_time;
        update sc, time_modify, time_metadata into file's metadata;
    }

Note that if the thing is already considered to be modified, then it
says to not request FATTR4_CHANGE. I have a related question around
this on the IETF list too.

> So, leave this patch as is, and I will deal with it
> later when we convert the callback XDR functions.
> 

Thanks, will do.
Chuck Lever Sept. 4, 2024, 5:45 p.m. UTC | #5
> On Sep 4, 2024, at 1:39 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
>> 
>>> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>> 
>>> 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,
>> 
>> Fwiw, it isn't purism about storing the bitmap, it's
>> purism about adopting machine-generated XDR marshaling/
>> unmarshaling code. The patch adds non-marshaling logic
>> in the encoder. Either we'll have to add a way to handle
>> that in xdrgen eventually, or we'll have to exclude this
>> encoder from machine generation. (This is a ways down the
>> road, of course)
>> 
> 
> Understood. I'll note that this function works with a nfs4_delegation
> pointer too, which is not part of the protocol spec.
> 
> Once we move to autogenerated code, we will always have a hand-rolled
> "glue" layer that morphs our internal representation of these sorts of
> objects into a form that the xdrgen code requires. Storing this info as
> a flag in the delegation makes more sense to me, as the glue layer
> should massage that into the needed form.

My thought was the existing proc functions would do
the massaging for us. But that's an abstract,
architectural kind of thought. I'm just starting to
integrate this idea into lockd.


--
Chuck Lever
NeilBrown Sept. 5, 2024, 1:44 a.m. UTC | #6
On Thu, 05 Sep 2024, Chuck Lever III wrote:
> 
> 
> > On Sep 4, 2024, at 1:39 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Wed, 2024-09-04 at 17:28 +0000, Chuck Lever III wrote:
> >> 
> >>> On Sep 4, 2024, at 12:58 PM, Jeff Layton <jlayton@kernel.org> wrote:
> >>> 
> >>> 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,
> >> 
> >> Fwiw, it isn't purism about storing the bitmap, it's
> >> purism about adopting machine-generated XDR marshaling/
> >> unmarshaling code. The patch adds non-marshaling logic
> >> in the encoder. Either we'll have to add a way to handle
> >> that in xdrgen eventually, or we'll have to exclude this
> >> encoder from machine generation. (This is a ways down the
> >> road, of course)
> >> 
> > 
> > Understood. I'll note that this function works with a nfs4_delegation
> > pointer too, which is not part of the protocol spec.
> > 
> > Once we move to autogenerated code, we will always have a hand-rolled
> > "glue" layer that morphs our internal representation of these sorts of
> > objects into a form that the xdrgen code requires. Storing this info as
> > a flag in the delegation makes more sense to me, as the glue layer
> > should massage that into the needed form.
> 
> My thought was the existing proc functions would do
> the massaging for us. But that's an abstract,
> architectural kind of thought. I'm just starting to
> integrate this idea into lockd.

So presumably xdrgen defines a bunch of structs.  The proc code fills
those in (on the stack?) and passes them to the generated xdr code which
encodes them to the network stream??  That seems reasonable.

We still don't want ncf_cb_bmap in struct nfs4_cb_fattr, we only need it
in the generated struct.

Thanks,
NeilBrown
diff mbox series

Patch

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;