diff mbox series

[2/2] nfsd: Record NFSv4 pre/post-op attributes as non-atomic

Message ID 20201201041427.756749-2-trondmy@kernel.org (mailing list archive)
State New
Headers show
Series [1/2] nfsd: Avoid /* Fallthrough */ | expand

Commit Message

trondmy@kernel.org Dec. 1, 2020, 4:14 a.m. UTC
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(-)

Comments

Chuck Lever Dec. 1, 2020, 4:10 p.m. UTC | #1
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
J. Bruce Fields Dec. 1, 2020, 7:35 p.m. UTC | #2
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
Trond Myklebust Dec. 1, 2020, 8:18 p.m. UTC | #3
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.
Trond Myklebust Dec. 1, 2020, 8:27 p.m. UTC | #4
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).
J. Bruce Fields Dec. 1, 2020, 8:53 p.m. UTC | #5
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.
Trond Myklebust Dec. 1, 2020, 8:59 p.m. UTC | #6
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.
J. Bruce Fields Dec. 1, 2020, 9:14 p.m. UTC | #7
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 mbox series

Patch

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;
 };