| Message ID | 20201201041427.756749-2-trondmy@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] nfsd: Avoid /* Fallthrough */ | expand |
Hi Trond- > On Nov 30, 2020, at 11:14 PM, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > For the case of NFSv4, specify to the client that the the pre/post-op > attributes were not recorded atomically with the main operation. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> I've applied this one provisionally to my tree for the next merge window. > --- > fs/nfs/export.c | 3 ++- > fs/nfsd/nfsfh.c | 4 ++++ > fs/nfsd/nfsfh.h | 5 +++++ > fs/nfsd/xdr4.h | 2 +- > include/linux/exportfs.h | 3 +++ > 5 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c > index 48b879cfe6e3..7412bb164fa7 100644 > --- a/fs/nfs/export.c > +++ b/fs/nfs/export.c > @@ -172,5 +172,6 @@ const struct export_operations nfs_export_ops = { > .fh_to_dentry = nfs_fh_to_dentry, > .get_parent = nfs_get_parent, > .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK| > - EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS, > + EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS| > + EXPORT_OP_NOATOMIC_ATTR, > }; > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index e80a7525561d..66f2ef67792a 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -301,6 +301,10 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > fhp->fh_export = exp; > > switch (rqstp->rq_vers) { > + case 4: > + if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR) > + fhp->fh_no_atomic_attr = true; > + break; > case 3: > if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC) > fhp->fh_no_wcc = true; > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > index 347d10aa6265..cb20c2cd3469 100644 > --- a/fs/nfsd/nfsfh.h > +++ b/fs/nfsd/nfsfh.h > @@ -36,6 +36,11 @@ typedef struct svc_fh { > bool fh_locked; /* inode locked by us */ > bool fh_want_write; /* remount protection taken */ > bool fh_no_wcc; /* no wcc data needed */ > + bool fh_no_atomic_attr; > + /* > + * wcc data is not atomic with > + * operation > + */ > int fh_flags; /* FH flags */ > #ifdef CONFIG_NFSD_V3 > bool fh_post_saved; /* post-op attrs saved */ > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index b4556e86e97c..a60ff5ce1a37 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -748,7 +748,7 @@ static inline void > set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > { > BUG_ON(!fhp->fh_pre_saved); > - cinfo->atomic = (u32)fhp->fh_post_saved; > + cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > cinfo->before_change = fhp->fh_pre_change; > cinfo->after_change = fhp->fh_post_change; > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index d93e8a6737bb..9f4d4bcbf251 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -217,6 +217,9 @@ struct export_operations { > #define EXPORT_OP_NOSUBTREECHK (0x2) /* no subtree checking */ > #define EXPORT_OP_CLOSE_BEFORE_UNLINK (0x4) /* close files before unlink */ > #define EXPORT_OP_REMOTE_FS (0x8) /* Filesystem is remote */ > +#define EXPORT_OP_NOATOMIC_ATTR (0x10) /* Filesystem cannot supply > + atomic attribute updates > + */ > unsigned long flags; > }; > > -- > 2.28.0 > -- Chuck Lever
On Mon, Nov 30, 2020 at 11:14:27PM -0500, trondmy@kernel.org wrote: > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > For the case of NFSv4, specify to the client that the the pre/post-op > attributes were not recorded atomically with the main operation. > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > --- > fs/nfs/export.c | 3 ++- > fs/nfsd/nfsfh.c | 4 ++++ > fs/nfsd/nfsfh.h | 5 +++++ > fs/nfsd/xdr4.h | 2 +- > include/linux/exportfs.h | 3 +++ > 5 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c > index 48b879cfe6e3..7412bb164fa7 100644 > --- a/fs/nfs/export.c > +++ b/fs/nfs/export.c > @@ -172,5 +172,6 @@ const struct export_operations nfs_export_ops = { > .fh_to_dentry = nfs_fh_to_dentry, > .get_parent = nfs_get_parent, > .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK| > - EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS, > + EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS| > + EXPORT_OP_NOATOMIC_ATTR, So I still don't understand why we need a new flag for this. Before you said it was because a filesystem might want to turn off the v4 atomic flag but still return v3 post-wcc attributes. But it seems that a) we have no example of a filesystem that wants to do that, b) it would violate the protocol anyway. Is that right? If so, then let's just stick to one flag for both. --b. > }; > diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c > index e80a7525561d..66f2ef67792a 100644 > --- a/fs/nfsd/nfsfh.c > +++ b/fs/nfsd/nfsfh.c > @@ -301,6 +301,10 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) > fhp->fh_export = exp; > > switch (rqstp->rq_vers) { > + case 4: > + if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR) > + fhp->fh_no_atomic_attr = true; > + break; > case 3: > if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC) > fhp->fh_no_wcc = true; > diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h > index 347d10aa6265..cb20c2cd3469 100644 > --- a/fs/nfsd/nfsfh.h > +++ b/fs/nfsd/nfsfh.h > @@ -36,6 +36,11 @@ typedef struct svc_fh { > bool fh_locked; /* inode locked by us */ > bool fh_want_write; /* remount protection taken */ > bool fh_no_wcc; /* no wcc data needed */ > + bool fh_no_atomic_attr; > + /* > + * wcc data is not atomic with > + * operation > + */ > int fh_flags; /* FH flags */ > #ifdef CONFIG_NFSD_V3 > bool fh_post_saved; /* post-op attrs saved */ > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h > index b4556e86e97c..a60ff5ce1a37 100644 > --- a/fs/nfsd/xdr4.h > +++ b/fs/nfsd/xdr4.h > @@ -748,7 +748,7 @@ static inline void > set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) > { > BUG_ON(!fhp->fh_pre_saved); > - cinfo->atomic = (u32)fhp->fh_post_saved; > + cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); > > cinfo->before_change = fhp->fh_pre_change; > cinfo->after_change = fhp->fh_post_change; > diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h > index d93e8a6737bb..9f4d4bcbf251 100644 > --- a/include/linux/exportfs.h > +++ b/include/linux/exportfs.h > @@ -217,6 +217,9 @@ struct export_operations { > #define EXPORT_OP_NOSUBTREECHK (0x2) /* no subtree checking */ > #define EXPORT_OP_CLOSE_BEFORE_UNLINK (0x4) /* close files before unlink */ > #define EXPORT_OP_REMOTE_FS (0x8) /* Filesystem is remote */ > +#define EXPORT_OP_NOATOMIC_ATTR (0x10) /* Filesystem cannot supply > + atomic attribute updates > + */ > unsigned long flags; > }; > > -- > 2.28.0
On Tue, 2020-12-01 at 14:35 -0500, J. Bruce Fields wrote: > On Mon, Nov 30, 2020 at 11:14:27PM -0500, trondmy@kernel.org wrote: > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > For the case of NFSv4, specify to the client that the the pre/post- > > op > > attributes were not recorded atomically with the main operation. > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > --- > > fs/nfs/export.c | 3 ++- > > fs/nfsd/nfsfh.c | 4 ++++ > > fs/nfsd/nfsfh.h | 5 +++++ > > fs/nfsd/xdr4.h | 2 +- > > include/linux/exportfs.h | 3 +++ > > 5 files changed, 15 insertions(+), 2 deletions(-) > > > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c > > index 48b879cfe6e3..7412bb164fa7 100644 > > --- a/fs/nfs/export.c > > +++ b/fs/nfs/export.c > > @@ -172,5 +172,6 @@ const struct export_operations nfs_export_ops = > > { > > .fh_to_dentry = nfs_fh_to_dentry, > > .get_parent = nfs_get_parent, > > .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK| > > - EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS, > > + EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS| > > + EXPORT_OP_NOATOMIC_ATTR, > > So I still don't understand why we need a new flag for this. > > Before you said it was because a filesystem might want to turn off > the > v4 atomic flag but still return v3 post-wcc attributes. > > But it seems that a) we have no example of a filesystem that wants to > do > that, b) it would violate the protocol anyway. > > Is that right? > > If so, then let's just stick to one flag for both. The "atomic" flag in NFSv4 is very specifically tied to a single attribute (the change attribute) and how it is collected in a very limited set of operations. The truth is that we probably _can_ eventually set it when re-exporting NFSv4 as NFSv4, assuming that the server actually supplies us with an atomic update. The same is not true of WCC. We might be able to supply knfsd with atomic updates for some operations, but certainly not for something like READ or WRITE. So I do think this needs to be separate flags. It is definitely describing completely different functionality.
On Tue, 2020-12-01 at 15:18 -0500, Trond Myklebust wrote: > On Tue, 2020-12-01 at 14:35 -0500, J. Bruce Fields wrote: > > On Mon, Nov 30, 2020 at 11:14:27PM -0500, trondmy@kernel.org wrote: > > > From: Trond Myklebust <trond.myklebust@hammerspace.com> > > > > > > For the case of NFSv4, specify to the client that the the > > > pre/post- > > > op > > > attributes were not recorded atomically with the main operation. > > > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com> > > > --- > > > fs/nfs/export.c | 3 ++- > > > fs/nfsd/nfsfh.c | 4 ++++ > > > fs/nfsd/nfsfh.h | 5 +++++ > > > fs/nfsd/xdr4.h | 2 +- > > > include/linux/exportfs.h | 3 +++ > > > 5 files changed, 15 insertions(+), 2 deletions(-) > > > > > > diff --git a/fs/nfs/export.c b/fs/nfs/export.c > > > index 48b879cfe6e3..7412bb164fa7 100644 > > > --- a/fs/nfs/export.c > > > +++ b/fs/nfs/export.c > > > @@ -172,5 +172,6 @@ const struct export_operations nfs_export_ops > > > = > > > { > > > .fh_to_dentry = nfs_fh_to_dentry, > > > .get_parent = nfs_get_parent, > > > .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK| > > > - > > > EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS, > > > + EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS > > > | > > > + EXPORT_OP_NOATOMIC_ATTR, > > > > So I still don't understand why we need a new flag for this. > > > > Before you said it was because a filesystem might want to turn off > > the > > v4 atomic flag but still return v3 post-wcc attributes. > > > > But it seems that a) we have no example of a filesystem that wants > > to > > do > > that, b) it would violate the protocol anyway. > > > > Is that right? > > > > If so, then let's just stick to one flag for both. > > The "atomic" flag in NFSv4 is very specifically tied to a single > attribute (the change attribute) and how it is collected in a very > limited set of operations. The truth is that we probably _can_ > eventually set it when re-exporting NFSv4 as NFSv4, assuming that the > server actually supplies us with an atomic update. > > The same is not true of WCC. We might be able to supply knfsd with > atomic updates for some operations, but certainly not for something > like READ or WRITE. > > So I do think this needs to be separate flags. It is definitely > describing completely different functionality. > Actually. The above is a good reason to just drop the EXPORT_OP_NOATOMIC_ATTR altogether. As far as I can tell, there is no need for it at all, since both the NFSv3 and NFSv4 client can supply atomic struct change_info4 in the cases where it is relevant (those cases being recording the changes to the parent directory/ies when doing CREATE, OPEN(O_CREAT), LINK, REMOVE and RENAME).
On Tue, Dec 01, 2020 at 08:27:38PM +0000, Trond Myklebust wrote: > As far as I can tell, there is no need for it at all, since both the > NFSv3 and NFSv4 client can supply atomic struct change_info4 in the > cases where it is relevant (those cases being recording the changes to > the parent directory/ies when doing CREATE, OPEN(O_CREAT), LINK, REMOVE > and RENAME). I was wondering about that. We'd need some additional interface to allow nfs to supply that stuff to nfsd, right? --b.
On Tue, 2020-12-01 at 15:53 -0500, bfields@fieldses.org wrote: > On Tue, Dec 01, 2020 at 08:27:38PM +0000, Trond Myklebust wrote: > > As far as I can tell, there is no need for it at all, since both > > the > > NFSv3 and NFSv4 client can supply atomic struct change_info4 in the > > cases where it is relevant (those cases being recording the changes > > to > > the parent directory/ies when doing CREATE, OPEN(O_CREAT), LINK, > > REMOVE > > and RENAME). > > I was wondering about that. We'd need some additional interface to > allow nfs to supply that stuff to nfsd, right? The only problem is the pre-op attributes, since those are supplied by the server but never recorded anywhere. Otherwise, all you really need is to hold the inode lock on the directory to prevent the client from making updates after the operation is done.
On Tue, Dec 01, 2020 at 08:59:00PM +0000, Trond Myklebust wrote: > On Tue, 2020-12-01 at 15:53 -0500, bfields@fieldses.org wrote: > > On Tue, Dec 01, 2020 at 08:27:38PM +0000, Trond Myklebust wrote: > > > As far as I can tell, there is no need for it at all, since both > > > the > > > NFSv3 and NFSv4 client can supply atomic struct change_info4 in the > > > cases where it is relevant (those cases being recording the changes > > > to > > > the parent directory/ies when doing CREATE, OPEN(O_CREAT), LINK, > > > REMOVE > > > and RENAME). > > > > I was wondering about that. We'd need some additional interface to > > allow nfs to supply that stuff to nfsd, right? > > The only problem is the pre-op attributes, since those are supplied by > the server but never recorded anywhere. Otherwise, all you really need > is to hold the inode lock on the directory to prevent the client from > making updates after the operation is done. We already have the lock over all those operations. I was worried that wouldn't prevent a getattr in another thread from updating the change attribute. Is that not possible? My only idea for returning extra information was a hack using kthread_data() as we're doing in nfsd_breaker_owns_lease(); nfs would call something like: void nfsd_set_cinfo(struct nfs4_change_info *cinfo) { struct svc_rqst *rqst; if (!i_am_nfsd()) return; rqst = kthread_data(current); rqst->cinfo = cinfo; } Then nfsd fetches it afterwards. Maybe there's a better way. --b.
diff --git a/fs/nfs/export.c b/fs/nfs/export.c index 48b879cfe6e3..7412bb164fa7 100644 --- a/fs/nfs/export.c +++ b/fs/nfs/export.c @@ -172,5 +172,6 @@ const struct export_operations nfs_export_ops = { .fh_to_dentry = nfs_fh_to_dentry, .get_parent = nfs_get_parent, .flags = EXPORT_OP_NOWCC|EXPORT_OP_NOSUBTREECHK| - EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS, + EXPORT_OP_CLOSE_BEFORE_UNLINK|EXPORT_OP_REMOTE_FS| + EXPORT_OP_NOATOMIC_ATTR, }; diff --git a/fs/nfsd/nfsfh.c b/fs/nfsd/nfsfh.c index e80a7525561d..66f2ef67792a 100644 --- a/fs/nfsd/nfsfh.c +++ b/fs/nfsd/nfsfh.c @@ -301,6 +301,10 @@ static __be32 nfsd_set_fh_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp) fhp->fh_export = exp; switch (rqstp->rq_vers) { + case 4: + if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOATOMIC_ATTR) + fhp->fh_no_atomic_attr = true; + break; case 3: if (dentry->d_sb->s_export_op->flags & EXPORT_OP_NOWCC) fhp->fh_no_wcc = true; diff --git a/fs/nfsd/nfsfh.h b/fs/nfsd/nfsfh.h index 347d10aa6265..cb20c2cd3469 100644 --- a/fs/nfsd/nfsfh.h +++ b/fs/nfsd/nfsfh.h @@ -36,6 +36,11 @@ typedef struct svc_fh { bool fh_locked; /* inode locked by us */ bool fh_want_write; /* remount protection taken */ bool fh_no_wcc; /* no wcc data needed */ + bool fh_no_atomic_attr; + /* + * wcc data is not atomic with + * operation + */ int fh_flags; /* FH flags */ #ifdef CONFIG_NFSD_V3 bool fh_post_saved; /* post-op attrs saved */ diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h index b4556e86e97c..a60ff5ce1a37 100644 --- a/fs/nfsd/xdr4.h +++ b/fs/nfsd/xdr4.h @@ -748,7 +748,7 @@ static inline void set_change_info(struct nfsd4_change_info *cinfo, struct svc_fh *fhp) { BUG_ON(!fhp->fh_pre_saved); - cinfo->atomic = (u32)fhp->fh_post_saved; + cinfo->atomic = (u32)(fhp->fh_post_saved && !fhp->fh_no_atomic_attr); cinfo->before_change = fhp->fh_pre_change; cinfo->after_change = fhp->fh_post_change; diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h index d93e8a6737bb..9f4d4bcbf251 100644 --- a/include/linux/exportfs.h +++ b/include/linux/exportfs.h @@ -217,6 +217,9 @@ struct export_operations { #define EXPORT_OP_NOSUBTREECHK (0x2) /* no subtree checking */ #define EXPORT_OP_CLOSE_BEFORE_UNLINK (0x4) /* close files before unlink */ #define EXPORT_OP_REMOTE_FS (0x8) /* Filesystem is remote */ +#define EXPORT_OP_NOATOMIC_ATTR (0x10) /* Filesystem cannot supply + atomic attribute updates + */ unsigned long flags; };