Received-SPF: pass (google.com: best guess record for domain of linux-nfs-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\))
Subject: Re: [PATCH 02/14] xattr: modify vfs_{set,remove}xattr for NFS server
 use
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200311195954.27117-3-fllinden@amazon.com>
Date:   Thu, 12 Mar 2020 12:23:44 -0400
Cc:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <C75E3B87-6A91-4258-949C-DDE323A77882@oracle.com>
References: <20200311195954.27117-1-fllinden@amazon.com>
 <20200311195954.27117-3-fllinden@amazon.com>
To:     Frank van der Linden <fllinden@amazon.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Mar 11, 2020, at 3:59 PM, Frank van der Linden =
<fllinden@amazon.com> wrote:
>=20
> To be called from the upcoming NFS server xattr code, the =
vfs_removexattr
> and vfs_setxattr need some modifications.
>=20
> First, they need to grow a _locked variant, since the NFS server code
> will call this with i_rwsem held. It needs to do that in fh_lock to be
> able to atomically provide the before and after change attributes.
>=20
> Second, RFC 8276 (NFSv4 extended attribute support) specifies that
> delegations should be recalled (8.4.2.4, 8.4.4.4) when a SETXATTR
> or REMOVEXATTR operation is performed. So, like with other fs
> operations, try to break the delegation. The _locked version of
> these operations will not wait for the delegation to be successfully
> broken, instead returning an error if it wasn't, so that the NFS
> server code can return NFS4ERR_DELAY to the client (similar to
> what e.g. vfs_link does).
>=20
> Signed-off-by: Frank van der Linden <fllinden@amazon.com>

Frank, I appreciate the verbosity of the patch descriptions, and
thanks very much for splitting the client and server work into
separate series.

[cel@klimt linux]$ scripts/get_maintainer.pl fs/xattr.c
Alexander Viro <viro@zeniv.linux.org.uk> (maintainer:FILESYSTEMS (VFS =
and infrastructure))
linux-fsdevel@vger.kernel.org (open list:FILESYSTEMS (VFS and =
infrastructure))
linux-kernel@vger.kernel.org (open list)
[cel@klimt linux]$=20

So patches like this one and 13/14 (or perhaps the whole series)
needs to be cc: linux-fsdevel@vger.kernel.org. At least those
two patches in particular will need an Acked-by: from viro.


> ---
> fs/xattr.c            | 63 =
+++++++++++++++++++++++++++++++++++++++++++++------
> include/linux/xattr.h |  2 ++
> 2 files changed, 58 insertions(+), 7 deletions(-)
>=20
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 90dd78f0eb27..58013bcbc333 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -204,10 +204,10 @@ int __vfs_setxattr_noperm(struct dentry *dentry, =
const char *name,
> 	return error;
> }
>=20
> -
> int
> -vfs_setxattr(struct dentry *dentry, const char *name, const void =
*value,
> -		size_t size, int flags)
> +__vfs_setxattr_locked(struct dentry *dentry, const char *name,
> +		const void *value, size_t size, int flags,
> +		struct inode **delegated_inode)

Since you will need to repost, please consider adding a Doxygen
comment in front of newly introduced global APIs. Such a comment
would be an appropriate place to add non-NFS-related explanatory
text you have provided in the patch description.

Goes for global APIs introduced in other patches too.


> {
> 	struct inode *inode =3D dentry->d_inode;
> 	int error;
> @@ -216,15 +216,40 @@ vfs_setxattr(struct dentry *dentry, const char =
*name, const void *value,
> 	if (error)
> 		return error;
>=20
> -	inode_lock(inode);
> 	error =3D security_inode_setxattr(dentry, name, value, size, =
flags);
> 	if (error)
> 		goto out;
>=20
> +	error =3D try_break_deleg(inode, delegated_inode);
> +	if (error)
> +		goto out;
> +
> 	error =3D __vfs_setxattr_noperm(dentry, name, value, size, =
flags);
>=20
> out:
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(__vfs_setxattr_locked);
> +
> +int
> +vfs_setxattr(struct dentry *dentry, const char *name, const void =
*value,
> +		size_t size, int flags)
> +{
> +	struct inode *inode =3D dentry->d_inode;
> +	struct inode *delegated_inode =3D NULL;
> +	int error;
> +
> +retry_deleg:
> +	inode_lock(inode);
> +	error =3D __vfs_setxattr_locked(dentry, name, value, size, =
flags,
> +	    &delegated_inode);
> 	inode_unlock(inode);
> +
> +	if (delegated_inode) {
> +		error =3D break_deleg_wait(&delegated_inode);
> +		if (!error)
> +			goto retry_deleg;
> +	}
> 	return error;
> }
> EXPORT_SYMBOL_GPL(vfs_setxattr);
> @@ -379,7 +404,8 @@ __vfs_removexattr(struct dentry *dentry, const =
char *name)
> EXPORT_SYMBOL(__vfs_removexattr);
>=20
> int
> -vfs_removexattr(struct dentry *dentry, const char *name)
> +__vfs_removexattr_locked(struct dentry *dentry, const char *name,
> +		struct inode **delegated_inode)
> {
> 	struct inode *inode =3D dentry->d_inode;
> 	int error;
> @@ -388,11 +414,14 @@ vfs_removexattr(struct dentry *dentry, const =
char *name)
> 	if (error)
> 		return error;
>=20
> -	inode_lock(inode);
> 	error =3D security_inode_removexattr(dentry, name);
> 	if (error)
> 		goto out;
>=20
> +	error =3D try_break_deleg(inode, delegated_inode);
> +	if (error)
> +		goto out;
> +
> 	error =3D __vfs_removexattr(dentry, name);
>=20
> 	if (!error) {
> @@ -401,12 +430,32 @@ vfs_removexattr(struct dentry *dentry, const =
char *name)
> 	}
>=20
> out:
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(__vfs_removexattr_locked);
> +
> +int
> +vfs_removexattr(struct dentry *dentry, const char *name)
> +{
> +	struct inode *inode =3D dentry->d_inode;
> +	struct inode *delegated_inode =3D NULL;
> +	int error;
> +
> +retry_deleg:
> +	inode_lock(inode);
> +	error =3D __vfs_removexattr_locked(dentry, name, =
&delegated_inode);
> 	inode_unlock(inode);
> +
> +	if (delegated_inode) {
> +		error =3D break_deleg_wait(&delegated_inode);
> +		if (!error)
> +			goto retry_deleg;
> +	}
> +
> 	return error;
> }
> EXPORT_SYMBOL_GPL(vfs_removexattr);
>=20
> -
> /*
>  * Extended attribute SET operations
>  */
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index 6dad031be3c2..3a71ad716da5 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -51,8 +51,10 @@ ssize_t vfs_getxattr(struct dentry *, const char *, =
void *, size_t);
> ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> int __vfs_setxattr(struct dentry *, struct inode *, const char *, =
const void *, size_t, int);
> int __vfs_setxattr_noperm(struct dentry *, const char *, const void *, =
size_t, int);
> +int __vfs_setxattr_locked(struct dentry *, const char *, const void =
*, size_t, int, struct inode **);
> int vfs_setxattr(struct dentry *, const char *, const void *, size_t, =
int);
> int __vfs_removexattr(struct dentry *, const char *);
> +int __vfs_removexattr_locked(struct dentry *, const char *, struct =
inode **);
> int vfs_removexattr(struct dentry *, const char *);
>=20
> ssize_t generic_listxattr(struct dentry *dentry, char *buffer, size_t =
buffer_size);
> --=20
> 2.16.6
>=20

--
Chuck Lever