Received-SPF: pass (google.com: domain of linux-nfs+bounces-3309-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 21 May 2024 10:02:58 -0400
From: Chuck Lever <chuck.lever@oracle.com>
To: Sagi Grimberg <sagi@grimberg.me>
Cc: linux-nfs@vger.kernel.org, Jeff Layton <jlayton@kernel.org>,
        Dan Aloni <dan.aloni@vastdata.com>, Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH rfc] nfs: propagate readlink errors in nfs_symlink_filler
Message-ID: <ZkypknHeBtBYJetq@tissot.1015granger.net>
References: <20240521125840.186618-1-sagi@grimberg.me>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240521125840.186618-1-sagi@grimberg.me>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?9+0TM8oAZ2qbmotyiYV2tbOzdsA3Q0FZ/5AXb4FpUyRxN8VQ76aYQFXyT+/A?=
 =?us-ascii?Q?LXtwNbMD6yTuHmKjKI/Lfsv7p9tlrPhCA+aLTdX3tlObbeyeC0ywWSgUAMBu?=
 =?us-ascii?Q?l6mTtZNN8boWVlswUkUtY80kuxr4ws7T40N1Aa6RYS6Onv3c3ikLoqbwneSD?=
 =?us-ascii?Q?1vo4MBGodq+ECmO0wR0VJnpaXP8G6rlNSaActZJ65lqGg7xoTbre9pETCK0S?=
 =?us-ascii?Q?G8tGk4+hwwcX+T9IvebcUaCsDCcjzYmJLYVsRkIGeRomkCHWN/hfVBjnpDAE?=
 =?us-ascii?Q?U/33Zu/FR9ZETeTpKLv+Xch/JSOK4bEwMXe2Povj5EIXpbm4/3wRbqZzsvYi?=
 =?us-ascii?Q?Gcxh9aP5XAzrWqFttzoQ4jAxpe+YGPciJMeQLYQPY6iOpagL9fGXeQJNtnC6?=
 =?us-ascii?Q?09sR/7io8IzPL/4rCDRKYX6YQI3GpjaCuYOcOZvlQhsz/bwOqcWLiXj7cD2x?=
 =?us-ascii?Q?gaUMjMAeWUFoIYf2yIiotze7tm6WIYvLpGasC/g2xNW/ROJ1Tpjj4kh9RNoR?=
 =?us-ascii?Q?ydIaC7jVs6duCNh9LCd+6+FM9L1HdCwR9dABbG1241jCUSeDHuHu0bp2XyOi?=
 =?us-ascii?Q?rdx9mF10q5gyjsqeH2VJNGHAzsOQD5wrUlgx4Gwlrlinqk3Z/PKyrHY5cOii?=
 =?us-ascii?Q?JqAjo2jm3WUHqA5sS+D/JSgvhuDHWNNf0cjOwptMlimz4NyKaad0u8R/Hokp?=
 =?us-ascii?Q?FYAbxfDg9spDYQ3TOelJAG1Vh2Dgc47de3Tn8Qu2vtUzvvQTFh6TofgNMQN4?=
 =?us-ascii?Q?lztMOrBV61POLiA713y6FQQ9FBnjhM5cVAO2klK5hiPIAQUPByNx9LrNC2if?=
 =?us-ascii?Q?jIEvSKH+uf2Pfocpu+cFDPuE84X4p0W8haH0D7xjlqAzZbY9Daxp9D8ChuJc?=
 =?us-ascii?Q?cc2D1H2gSqPKdNHZIQBn3Hjb+pMsCS/W5wTBbHkMZ9H3fZAkGTg9jyB4xpHI?=
 =?us-ascii?Q?MUDBzWp7z57TNg+RZMc2qzaw92EHOE5ui40/CS8VBDQctmsGMK2m4rn7TBjd?=
 =?us-ascii?Q?d79/mrjtnQYncdFZy4XQo3nk9krP1+v1Gerb9oabmxqUhMU9ASlPaxvtSgCA?=
 =?us-ascii?Q?ryB7WZpfKg+a5agbMhiPq25c0YGub3H14sQywfeyZmCGhOAIW/dLhjCvIGaF?=
 =?us-ascii?Q?Riy8ySsCEJF1UXhDTg5T/8Va4s5+K0jvolW1M5OTjqBu+AQumzJsd2MdetWL?=
 =?us-ascii?Q?DhWHKqnr3f/acLb86eLwvV471eJfYtmee0Zq5HNZy+5oVBl8b8pocI58lyLQ?=
 =?us-ascii?Q?rmnmwZVVVoIWBj51imC8OAig0hbYlRLa/viPr1/aV0dlBMsnohW8Q4bmCInn?=
 =?us-ascii?Q?n/MvBxi++gh7+CTHFxkZJ+wAER/1YzmGIe9/7Dn/yzPXBU4Gfs+h44LybEG1?=
 =?us-ascii?Q?O6MvpFnmInFZJdI3jcY3csTJu2bnetbcPlxm56lo+VI4ZBywDLWV6N9ux6XE?=
 =?us-ascii?Q?1LruOOHndltu54fp52Ao176m1K1oZq4maUWOTDQ+GUS71KJ+RUcRuw/myIE5?=
 =?us-ascii?Q?0Z1pKMUPJ9ESvz+Gth4WgTnSYrkqqg/7/culhJNjXDBGULxnHvX2ZtIWBcvj?=
 =?us-ascii?Q?4PPa85hCbPPqAzu/sCZ3gSY55Akku+PcKPVxSfWHpaSOiC7Gdv8oy15vAE00?=
 =?us-ascii?Q?7A=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	EhFYHqv3FWX2KawpAnf/+gRlfjM1Nf5IRvZ08s/QvdO6wn1etFhwPWCjt+lSEaGF/fVt8WW3Yp5b0Lr0m8CaT6T/DJyJWo4PkBaopFFc4ovnRjaDDdRIh6cp9N+SKRp5OAp+UxQF07T6jvH+plozh4Mm0tH69y2bEf3dqmR2C0TtIAQ6k93XFJg9P0ZnivCkWTPpj8ZiQGt5N5LzSFCNlv+itMbIXpduaoqaQvIhxbq8CLVpGaDcYIX6u1tHftGjP7E+pTszyTWs+ZjqNZc/029jigIMOfpfjQ4i/qM7HCtjOL852aEqMylqdGziyS17N0pJjtQcLbdI7nIPp5DZ39Obw4cJSgtYKzHX14S33SmG3SF30KmFdRK2U3AcsttrAqwWpPnhk7DJb5mnKjYHCvF86u/aXcRPLPrrpxxyRC/6qpnwIZz3DAidPZtIKJQ+StDThhvnaoDCx4vewt2jHqPlHfMWIshcnuidtPJGgiW53J7iiiqILhPEoev94qGA5yHPebxfGi2OKrseOP6OJ3S6L5FMTlOO+15dqJwir2XzG5FqeMF9Cucnqe5CDirNa3C9ZsJgGG2OnZPXPLd80PYMUF2WVYqlPz2H0vv9TKc=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e6d1c752-9bce-4207-a817-08dc799eba2d
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2024 14:03:01.6239
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bl2KY7CcqOm9VHqUtKeKl+4Rwa+4HoB0LT9S6uwPZ6FoN2mJYRqDZEAda0AisvGcexI1z8shfcI1iw81/lmnqg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR10MB5689
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.650,FMLib:17.12.28.16
 definitions=2024-05-21_08,2024-05-21_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 mlxscore=0 mlxlogscore=989
 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2405210105
X-Proofpoint-GUID: zInKXdqqPHBqNmrcuYljUoK1K9TZ8y9q
X-Proofpoint-ORIG-GUID: zInKXdqqPHBqNmrcuYljUoK1K9TZ8y9q

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" ?


> (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" ?


> 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.


> 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?

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.

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.


>  fs/nfs/symlink.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfs/symlink.c b/fs/nfs/symlink.c
> index 0e27a2e4e68b..13818129d268 100644
> --- a/fs/nfs/symlink.c
> +++ b/fs/nfs/symlink.c
> @@ -41,7 +41,7 @@ static int nfs_symlink_filler(struct file *file, struct folio *folio)
>  error:
>  	folio_set_error(folio);
>  	folio_unlock(folio);
> -	return -EIO;
> +	return error;
>  }
>  
>  static const char *nfs_get_link(struct dentry *dentry,
> -- 
> 2.40.1
> 

-- 
Chuck Lever