Received-SPF: pass (google.com: domain of linux-nfs+bounces-3314-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <c43d096d-bde0-4168-931f-0df96d8b0bed@grimberg.me>
Date: Tue, 21 May 2024 17:59:18 +0300
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler
To: Chuck Lever <chuck.lever@oracle.com>
Cc: linux-nfs@vger.kernel.org, Jeff Layton <jlayton@kernel.org>,
 Dan Aloni <dan.aloni@vastdata.com>, Christoph Hellwig <hch@lst.de>
References: <20240521125840.186618-1-sagi@grimberg.me>
 <ZkypknHeBtBYJetq@tissot.1015granger.net>
Content-Language: he-IL, en-US
From: Sagi Grimberg <sagi@grimberg.me>
In-Reply-To: <ZkypknHeBtBYJetq@tissot.1015granger.net>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 21/05/2024 17:02, Chuck Lever wrote:
> On Tue, May 21, 2024 at 03:58:40PM +0300, Sagi Grimberg wrote:
>> There is an inherent race where a symlink file may have been overriden
> Nit: Do you mean "overwritten" ?

Yes.

>
>
>> (by a different client) between lookup and readlink, resulting in a
>> spurious EIO error returned to userspace. Fix this by propagating back
>> ESTALE errors such that the vfs will retry the lookup/get_link (similar
>> to nfs4_file_open) at least once.
>>
>> Cc: Dan Aloni <dan.aloni@vastdata.com>
>> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>> ---
>> Note that with this change the vfs should retry once for
>> ESTALE errors. However with an artificial reproducer of high
>> frequency symlink overrides, nothing prevents the retry to
> Nit: "overwrites" ?

Yes.

>
>
>> also encounter ESTALE, propagating the error back to userspace.
>> The man pages for openat/readlinkat do not list an ESTALE errno.
> Speaking only as a community member, I consider that an undesirable
> behavior regression. IMO it's a bug for a system call to return an
> errno that isn't documented. That's likely why this logic has worked
> this way for forever.

Well, if this is an issue, it would be paired with a vfs change that
checks for the error-code of the retry and convert it back.
Something like:
--
diff --git a/fs/namei.c b/fs/namei.c
index ceb9ddf8dfdd..9a06408bb52f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3812,6 +3812,8 @@ static struct file *path_openat(struct nameidata *nd,
                 else
                         error = -ESTALE;
         }
+       if (error == -ESTALE && (flags & LOOKUP_REVAL))
+               error = -EIO;
         return ERR_PTR(error);
  }
--

But we'd need to check with Al for this type of a  change...

>
>
>> An alternative attempt (implemented by Dan) was a local retry loop
>> in nfs_get_link(), if this is an applicable approach, Dan can
>> share his patch instead.
> I'm not entirely convinced by your patch description that returning
> an EIO on occasion is a problem. Is it reasonable for the app to
> expect that readlinkat() will /never/ fail?

Maybe not never, but its fairly easy to encounter, and it was definitely
observed in the wild.

>
> Making symlink semantics more atomic on NFS mounts is probably a
> good goal. But IMO the proposed change by itself isn't going to get
> you that with high reliability and few or no undesirable side
> effects.

What undesirable effects?

>
> Note that NFS client-side patches should be sent To: Trond, Anna,
> and Cc: linux-nfs@ . Trond and Anna need to weigh in on this.

Yes, it was sent to linux-nfs so I expect Trond and Anna to see it, you
and Jeff were CC'd because we briefly discussed about this last week at
LSFMM.