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