On Fri, 2021-05-28 at 10:30 -0700, dai.ngo@xxxxxxxxxx wrote:
> Hi Trond,
>
> Just a reminder that this patch is ready for your review.
Sorry. I missed that update for some reason.
>
> Thanks,
> -Dai
>
> On 5/19/21 2:15 PM, Dai Ngo wrote:
> > Currently if __nfs4_proc_set_acl fails with NFS4ERR_BADOWNER it
> > re-enables the idmapper by clearing NFS_CAP_UIDGID_NOMAP before
> > retrying again. The NFS_CAP_UIDGID_NOMAP remains cleared even if
> > the retry fails. This causes problem for subsequent setattr
> > requests for v4 server that does not have idmapping configured.
> >
> > This patch modifies nfs4_proc_set_acl to detect NFS4ERR_BADOWNER
> > and NFS4ERR_BADNAME and skips the retry, since the kernel isn't
> > involved in encoding the ACEs, and return -EINVAL.
> >
> > Steps to reproduce the problem:
> >
> > # mount -o vers=4.1,sec=sys server:/export/test /tmp/mnt
> > # touch /tmp/mnt/file1
> > # chown 99 /tmp/mnt/file1
> > # nfs4_setfacl -a A::unknown.user@xxxxxxx:wrtncy /tmp/mnt/file1
> > Failed setxattr operation: Invalid argument
> > # chown 99 /tmp/mnt/file1
> > chown: changing ownership of ‘/tmp/mnt/file1’: Invalid argument
> > # umount /tmp/mnt
> > # mount -o vers=4.1,sec=sys server:/export/test /tmp/mnt
> > # chown 99 /tmp/mnt/file1
> > #
> >
> > v2: detect NFS4ERR_BADOWNER and NFS4ERR_BADNAME and skip retry
> > in nfs4_proc_set_acl.
> > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> > ---
> > fs/nfs/nfs4proc.c | 8 ++++++++
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 87d04f2c9385..4e052c7f0614 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -5968,6 +5968,14 @@ static int nfs4_proc_set_acl(struct inode
> > *inode, const void *buf, size_t buflen
> > do {
> > err = __nfs4_proc_set_acl(inode, buf, buflen);
> > trace_nfs4_set_acl(inode, err);
> > + if (err == -NFS4ERR_BADOWNER || err == -
> > NFS4ERR_BADNAME) {
> > + /*
> > + * no need to retry since the kernel
> > + * isn't involved in encoding the ACEs.
> > + */
> > + err = -EINVAL;
> > + break;
> > + }
> > err = nfs4_handle_exception(NFS_SERVER(inode), err,
> > &exception);
> > } while (exception.retry);
Yes, this looks OK.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
