[PATCH v3] ceph: fail the open_by_handle_at() if the dentry is being unlinked

[PATCH v3] ceph: fail the open_by_handle_at() if the dentry is being unlinked

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

When unlinking a file the kclient will send a unlink request to MDS
by holding the dentry reference, and then the MDS will return 2 replies,
which are unsafe reply and a deferred safe reply.

After the unsafe reply received the kernel will return and succeed
the unlink request to user space apps.

Only when the safe reply received the dentry's reference will be
released. Or the dentry will only be unhashed from dcache. But when
the open_by_handle_at() begins to open the unlinked files it will
succeed.

The inode->i_count couldn't be used to check whether the inode is
opened or not.

URL: https://tracker.ceph.com/issues/56524
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---

V3:
- The inode->i_count couldn't be correctly indicate that whether the
  file is opened or not.

V2:
- If the dentry was released and inode is evicted such as by dropping
  the caches, it will allocate a new dentry, which is also unhashed.

 fs/ceph/export.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/ceph/export.c b/fs/ceph/export.c
index 0ebf2bd93055..8559990a59a5 100644
--- a/fs/ceph/export.c
+++ b/fs/ceph/export.c
@@ -182,6 +182,7 @@ struct inode *ceph_lookup_inode(struct super_block *sb, u64 ino)
 static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
 {
 	struct inode *inode = __lookup_inode(sb, ino);
+	struct ceph_inode_info *ci = ceph_inode(inode);
 	int err;
 
 	if (IS_ERR(inode))
@@ -193,7 +194,7 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
 		return ERR_PTR(err);
 	}
 	/* -ESTALE if inode as been unlinked and no file is open */
-	if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) {
+	if ((inode->i_nlink == 0) && !__ceph_is_file_opened(ci)) {
 		iput(inode);
 		return ERR_PTR(-ESTALE);
 	}
-- 
2.36.0.rc1

Re: [PATCH v3] ceph: fail the open_by_handle_at() if the dentry is being unlinked

[Jeff Layton]

On Wed, 2022-08-31 at 10:16 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> When unlinking a file the kclient will send a unlink request to MDS
> by holding the dentry reference, and then the MDS will return 2 replies,
> which are unsafe reply and a deferred safe reply.
> 
> After the unsafe reply received the kernel will return and succeed
> the unlink request to user space apps.
> 
> Only when the safe reply received the dentry's reference will be
> released. Or the dentry will only be unhashed from dcache. But when
> the open_by_handle_at() begins to open the unlinked files it will
> succeed.
> 
> The inode->i_count couldn't be used to check whether the inode is
> opened or not.
> 
> URL: https://tracker.ceph.com/issues/56524
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
> 
> V3:
> - The inode->i_count couldn't be correctly indicate that whether the
>   file is opened or not.
> 
> V2:
> - If the dentry was released and inode is evicted such as by dropping
>   the caches, it will allocate a new dentry, which is also unhashed.
> 
>  fs/ceph/export.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> index 0ebf2bd93055..8559990a59a5 100644
> --- a/fs/ceph/export.c
> +++ b/fs/ceph/export.c
> @@ -182,6 +182,7 @@ struct inode *ceph_lookup_inode(struct super_block *sb, u64 ino)
>  static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>  {
>  	struct inode *inode = __lookup_inode(sb, ino);
> +	struct ceph_inode_info *ci = ceph_inode(inode);
>  	int err;
>  
>  	if (IS_ERR(inode))
> @@ -193,7 +194,7 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>  		return ERR_PTR(err);
>  	}
>  	/* -ESTALE if inode as been unlinked and no file is open */
> -	if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) {
> +	if ((inode->i_nlink == 0) && !__ceph_is_file_opened(ci)) {
>  		iput(inode);
>  		return ERR_PTR(-ESTALE);
>  	}

Looks reasonable

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3] ceph: fail the open_by_handle_at() if the dentry is being unlinked

[Luís Henriques]

On Wed, Aug 31, 2022 at 10:16:17AM +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> When unlinking a file the kclient will send a unlink request to MDS
> by holding the dentry reference, and then the MDS will return 2 replies,
> which are unsafe reply and a deferred safe reply.
> 
> After the unsafe reply received the kernel will return and succeed
> the unlink request to user space apps.
> 
> Only when the safe reply received the dentry's reference will be
> released. Or the dentry will only be unhashed from dcache. But when
> the open_by_handle_at() begins to open the unlinked files it will
> succeed.
> 
> The inode->i_count couldn't be used to check whether the inode is
> opened or not.
> 
> URL: https://tracker.ceph.com/issues/56524
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
> 
> V3:
> - The inode->i_count couldn't be correctly indicate that whether the
>   file is opened or not.
> 
> V2:
> - If the dentry was released and inode is evicted such as by dropping
>   the caches, it will allocate a new dentry, which is also unhashed.
> 
>  fs/ceph/export.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/ceph/export.c b/fs/ceph/export.c
> index 0ebf2bd93055..8559990a59a5 100644
> --- a/fs/ceph/export.c
> +++ b/fs/ceph/export.c
> @@ -182,6 +182,7 @@ struct inode *ceph_lookup_inode(struct super_block *sb, u64 ino)
>  static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>  {
>  	struct inode *inode = __lookup_inode(sb, ino);
> +	struct ceph_inode_info *ci = ceph_inode(inode);
>  	int err;
>  
>  	if (IS_ERR(inode))
> @@ -193,7 +194,7 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>  		return ERR_PTR(err);
>  	}
>  	/* -ESTALE if inode as been unlinked and no file is open */
> -	if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) {
> +	if ((inode->i_nlink == 0) && !__ceph_is_file_opened(ci)) {
>  		iput(inode);
>  		return ERR_PTR(-ESTALE);
>  	}
> -- 
> 2.36.0.rc1
> 

Thanks, this seems be correct.  I was able to reproduce this locally, and
I can confirm this patch fixes it.  (Although I had this fixed this in the
past with 878dabb64117 and at that time it looked like it was fixed too.)

Feel free to add my:

Tested-by: Luís Henriques <lhenriques@suse.de>
Reviewed-by: Luís Henriques <lhenriques@suse.de>

Cheers,
--
Luís

Re: [PATCH v3] ceph: fail the open_by_handle_at() if the dentry is being unlinked

[Xiubo Li]

On 8/31/22 10:13 PM, Luís Henriques wrote:
> On Wed, Aug 31, 2022 at 10:16:17AM +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> When unlinking a file the kclient will send a unlink request to MDS
>> by holding the dentry reference, and then the MDS will return 2 replies,
>> which are unsafe reply and a deferred safe reply.
>>
>> After the unsafe reply received the kernel will return and succeed
>> the unlink request to user space apps.
>>
>> Only when the safe reply received the dentry's reference will be
>> released. Or the dentry will only be unhashed from dcache. But when
>> the open_by_handle_at() begins to open the unlinked files it will
>> succeed.
>>
>> The inode->i_count couldn't be used to check whether the inode is
>> opened or not.
>>
>> URL: https://tracker.ceph.com/issues/56524
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>>
>> V3:
>> - The inode->i_count couldn't be correctly indicate that whether the
>>    file is opened or not.
>>
>> V2:
>> - If the dentry was released and inode is evicted such as by dropping
>>    the caches, it will allocate a new dentry, which is also unhashed.
>>
>>   fs/ceph/export.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/ceph/export.c b/fs/ceph/export.c
>> index 0ebf2bd93055..8559990a59a5 100644
>> --- a/fs/ceph/export.c
>> +++ b/fs/ceph/export.c
>> @@ -182,6 +182,7 @@ struct inode *ceph_lookup_inode(struct super_block *sb, u64 ino)
>>   static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>>   {
>>   	struct inode *inode = __lookup_inode(sb, ino);
>> +	struct ceph_inode_info *ci = ceph_inode(inode);
>>   	int err;
>>   
>>   	if (IS_ERR(inode))
>> @@ -193,7 +194,7 @@ static struct dentry *__fh_to_dentry(struct super_block *sb, u64 ino)
>>   		return ERR_PTR(err);
>>   	}
>>   	/* -ESTALE if inode as been unlinked and no file is open */
>> -	if ((inode->i_nlink == 0) && (atomic_read(&inode->i_count) == 1)) {
>> +	if ((inode->i_nlink == 0) && !__ceph_is_file_opened(ci)) {
>>   		iput(inode);
>>   		return ERR_PTR(-ESTALE);
>>   	}
>> -- 
>> 2.36.0.rc1
>>
> Thanks, this seems be correct.  I was able to reproduce this locally, and
> I can confirm this patch fixes it.  (Although I had this fixed this in the
> past with 878dabb64117 and at that time it looked like it was fixed too.)

Yeah, this is much harder to reproduce since you last two fixes about 
this. Locally I need to make a change in the xfstests source code to 
trigger it easier or I may need half day or more to see it.

> Feel free to add my:
>
> Tested-by: Luís Henriques <lhenriques@suse.de>
> Reviewed-by: Luís Henriques <lhenriques@suse.de>

Thanks Luis and Jeff.

Updated this.

-- Xiubo


> Cheers,
> --
> Luís
>