* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-01 10:38 ` Jeff Layton
@ 2022-07-04 1:13 ` Xiubo Li
2022-07-04 2:10 ` Matthew Wilcox
2022-07-04 6:58 ` Xiubo Li
2022-07-05 13:21 ` David Howells
2 siblings, 1 reply; 11+ messages in thread
From: Xiubo Li @ 2022-07-04 1:13 UTC (permalink / raw)
To: Jeff Layton, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs
On 7/1/22 6:38 PM, Jeff Layton wrote:
> On Fri, 2022-07-01 at 10:29 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The lower layer filesystem should always make sure the folio is
>> locked and do the unlock and put the folio in netfs layer.
>>
>> URL: https://tracker.ceph.com/issues/56423
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> fs/netfs/buffered_read.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
>> index 42f892c5712e..257fd37c2461 100644
>> --- a/fs/netfs/buffered_read.c
>> +++ b/fs/netfs/buffered_read.c
>> @@ -351,8 +351,11 @@ int netfs_write_begin(struct netfs_inode *ctx,
>> ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
>> if (ret < 0) {
>> trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
>> - if (ret == -EAGAIN)
>> + if (ret == -EAGAIN) {
>> + folio_unlock(folio);
>> + folio_put(folio);
>> goto retry;
>> + }
>> goto error;
>> }
>> }
> I don't know here... I think it might be better to just expect that when
> this function returns an error that the folio has already been unlocked.
> Doing it this way will mean that you will lock and unlock the folio a
> second time for no reason.
>
> Maybe something like this instead?
>
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 42f892c5712e..8ae7b0f4c909 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -353,7 +353,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
> if (ret == -EAGAIN)
> goto retry;
> - goto error;
> + goto error_unlocked;
> }
> }
>
> @@ -418,6 +418,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> error:
> folio_unlock(folio);
> folio_put(folio);
> +error_unlocked:
> _leave(" = %d", ret);
> return ret;
> }
Then the "afs" won't work correctly:
377 static int afs_check_write_begin(struct file *file, loff_t pos,
unsigned len,
378 struct folio *folio, void **_fsdata)
379 {
380 struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
381
382 return test_bit(AFS_VNODE_DELETED, &vnode->flags) ? -ESTALE : 0;
383 }
The "afs" does nothing with the folio lock.
-- Xiubo
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-04 1:13 ` Xiubo Li
@ 2022-07-04 2:10 ` Matthew Wilcox
2022-07-04 2:40 ` Xiubo Li
0 siblings, 1 reply; 11+ messages in thread
From: Matthew Wilcox @ 2022-07-04 2:10 UTC (permalink / raw)
To: Xiubo Li
Cc: Jeff Layton, idryomov, dhowells, vshankar, linux-kernel,
ceph-devel, keescook, linux-fsdevel, linux-cachefs
On Mon, Jul 04, 2022 at 09:13:44AM +0800, Xiubo Li wrote:
> On 7/1/22 6:38 PM, Jeff Layton wrote:
> > I don't know here... I think it might be better to just expect that when
> > this function returns an error that the folio has already been unlocked.
> > Doing it this way will mean that you will lock and unlock the folio a
> > second time for no reason.
> >
> > Maybe something like this instead?
> >
> > diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> > index 42f892c5712e..8ae7b0f4c909 100644
> > --- a/fs/netfs/buffered_read.c
> > +++ b/fs/netfs/buffered_read.c
> > @@ -353,7 +353,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> > trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
> > if (ret == -EAGAIN)
> > goto retry;
> > - goto error;
> > + goto error_unlocked;
> > }
> > }
> > @@ -418,6 +418,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> > error:
> > folio_unlock(folio);
> > folio_put(folio);
> > +error_unlocked:
> > _leave(" = %d", ret);
> > return ret;
> > }
>
> Then the "afs" won't work correctly:
>
> 377 static int afs_check_write_begin(struct file *file, loff_t pos, unsigned
> len,
> 378 struct folio *folio, void **_fsdata)
> 379 {
> 380 struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
> 381
> 382 return test_bit(AFS_VNODE_DELETED, &vnode->flags) ? -ESTALE : 0;
> 383 }
>
> The "afs" does nothing with the folio lock.
It's OK to fix AFS too.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-04 2:10 ` Matthew Wilcox
@ 2022-07-04 2:40 ` Xiubo Li
0 siblings, 0 replies; 11+ messages in thread
From: Xiubo Li @ 2022-07-04 2:40 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Jeff Layton, idryomov, dhowells, vshankar, linux-kernel,
ceph-devel, keescook, linux-fsdevel, linux-cachefs
On 7/4/22 10:10 AM, Matthew Wilcox wrote:
> On Mon, Jul 04, 2022 at 09:13:44AM +0800, Xiubo Li wrote:
>> On 7/1/22 6:38 PM, Jeff Layton wrote:
>>> I don't know here... I think it might be better to just expect that when
>>> this function returns an error that the folio has already been unlocked.
>>> Doing it this way will mean that you will lock and unlock the folio a
>>> second time for no reason.
>>>
>>> Maybe something like this instead?
>>>
>>> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
>>> index 42f892c5712e..8ae7b0f4c909 100644
>>> --- a/fs/netfs/buffered_read.c
>>> +++ b/fs/netfs/buffered_read.c
>>> @@ -353,7 +353,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
>>> trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
>>> if (ret == -EAGAIN)
>>> goto retry;
>>> - goto error;
>>> + goto error_unlocked;
>>> }
>>> }
>>> @@ -418,6 +418,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
>>> error:
>>> folio_unlock(folio);
>>> folio_put(folio);
>>> +error_unlocked:
>>> _leave(" = %d", ret);
>>> return ret;
>>> }
>> Then the "afs" won't work correctly:
>>
>> 377 static int afs_check_write_begin(struct file *file, loff_t pos, unsigned
>> len,
>> 378 struct folio *folio, void **_fsdata)
>> 379 {
>> 380 struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
>> 381
>> 382 return test_bit(AFS_VNODE_DELETED, &vnode->flags) ? -ESTALE : 0;
>> 383 }
>>
>> The "afs" does nothing with the folio lock.
> It's OK to fix AFS too.
>
Okay, will fix it. Thanks!
-- Xiubo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-01 10:38 ` Jeff Layton
2022-07-04 1:13 ` Xiubo Li
@ 2022-07-04 6:58 ` Xiubo Li
2022-07-05 13:21 ` David Howells
2 siblings, 0 replies; 11+ messages in thread
From: Xiubo Li @ 2022-07-04 6:58 UTC (permalink / raw)
To: Jeff Layton, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs
On 7/1/22 6:38 PM, Jeff Layton wrote:
> On Fri, 2022-07-01 at 10:29 +0800, xiubli@redhat.com wrote:
>> From: Xiubo Li <xiubli@redhat.com>
>>
>> The lower layer filesystem should always make sure the folio is
>> locked and do the unlock and put the folio in netfs layer.
>>
>> URL: https://tracker.ceph.com/issues/56423
>> Signed-off-by: Xiubo Li <xiubli@redhat.com>
>> ---
>> fs/netfs/buffered_read.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
>> index 42f892c5712e..257fd37c2461 100644
>> --- a/fs/netfs/buffered_read.c
>> +++ b/fs/netfs/buffered_read.c
>> @@ -351,8 +351,11 @@ int netfs_write_begin(struct netfs_inode *ctx,
>> ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
>> if (ret < 0) {
>> trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
>> - if (ret == -EAGAIN)
>> + if (ret == -EAGAIN) {
>> + folio_unlock(folio);
>> + folio_put(folio);
>> goto retry;
>> + }
>> goto error;
>> }
>> }
> I don't know here... I think it might be better to just expect that when
> this function returns an error that the folio has already been unlocked.
> Doing it this way will mean that you will lock and unlock the folio a
> second time for no reason.
>
> Maybe something like this instead?
>
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 42f892c5712e..8ae7b0f4c909 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -353,7 +353,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
> if (ret == -EAGAIN)
> goto retry;
> - goto error;
> + goto error_unlocked;
> }
> }
>
> @@ -418,6 +418,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
> error:
> folio_unlock(folio);
> folio_put(folio);
> +error_unlocked:
Should we also put the folio in ceph and afs ? Won't it introduce
something like use-after-free bug ?
Maybe we should unlock it in ceph and afs and put it in netfs layer.
-- Xiubo
> _leave(" = %d", ret);
> return ret;
> }
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-01 10:38 ` Jeff Layton
2022-07-04 1:13 ` Xiubo Li
2022-07-04 6:58 ` Xiubo Li
@ 2022-07-05 13:21 ` David Howells
2022-07-05 13:41 ` Jeff Layton
2022-07-06 0:58 ` Xiubo Li
2 siblings, 2 replies; 11+ messages in thread
From: David Howells @ 2022-07-05 13:21 UTC (permalink / raw)
To: Jeff Layton
Cc: dhowells, xiubli, idryomov, vshankar, linux-kernel, ceph-devel,
willy, keescook, linux-fsdevel, linux-cachefs
Jeff Layton <jlayton@kernel.org> wrote:
> I don't know here... I think it might be better to just expect that when
> this function returns an error that the folio has already been unlocked.
> Doing it this way will mean that you will lock and unlock the folio a
> second time for no reason.
I seem to remember there was some reason you wanted the folio unlocking and
putting. I guess you need to drop the ref to flush it.
Would it make sense for ->check_write_begin() to be passed a "struct folio
**folio" rather than "struct folio *folio" and then the filesystem can clear
*folio if it disposes of the page?
David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-05 13:21 ` David Howells
@ 2022-07-05 13:41 ` Jeff Layton
2022-07-06 0:58 ` Xiubo Li
1 sibling, 0 replies; 11+ messages in thread
From: Jeff Layton @ 2022-07-05 13:41 UTC (permalink / raw)
To: David Howells
Cc: xiubli, idryomov, vshankar, linux-kernel, ceph-devel, willy,
keescook, linux-fsdevel, linux-cachefs
On Tue, 2022-07-05 at 14:21 +0100, David Howells wrote:
> Jeff Layton <jlayton@kernel.org> wrote:
>
> > I don't know here... I think it might be better to just expect that when
> > this function returns an error that the folio has already been unlocked.
> > Doing it this way will mean that you will lock and unlock the folio a
> > second time for no reason.
>
> I seem to remember there was some reason you wanted the folio unlocking and
> putting. I guess you need to drop the ref to flush it.
>
> Would it make sense for ->check_write_begin() to be passed a "struct folio
> **folio" rather than "struct folio *folio" and then the filesystem can clear
> *folio if it disposes of the page?
>
I'd be OK with that too.
--
Jeff Layton <jlayton@kernel.org>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-05 13:21 ` David Howells
2022-07-05 13:41 ` Jeff Layton
@ 2022-07-06 0:58 ` Xiubo Li
1 sibling, 0 replies; 11+ messages in thread
From: Xiubo Li @ 2022-07-06 0:58 UTC (permalink / raw)
To: David Howells, Jeff Layton
Cc: idryomov, vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs
On 7/5/22 9:21 PM, David Howells wrote:
> Jeff Layton <jlayton@kernel.org> wrote:
>
>> I don't know here... I think it might be better to just expect that when
>> this function returns an error that the folio has already been unlocked.
>> Doing it this way will mean that you will lock and unlock the folio a
>> second time for no reason.
> I seem to remember there was some reason you wanted the folio unlocking and
> putting. I guess you need to drop the ref to flush it.
>
> Would it make sense for ->check_write_begin() to be passed a "struct folio
> **folio" rather than "struct folio *folio" and then the filesystem can clear
> *folio if it disposes of the page?
Yeah, this also sounds good to me.
-- Xiubo
> David
>
^ permalink raw reply [flat|nested] 11+ messages in thread