* [PATCH 0/2] netfs, ceph: fix the crash when unlocking the folio
@ 2022-07-01 2:29 xiubli
2022-07-01 2:29 ` [PATCH 1/2] netfs: release the folio lock and put the folio before retrying xiubli
2022-07-01 2:29 ` [PATCH 2/2] ceph: do not release the folio lock in kceph xiubli
0 siblings, 2 replies; 11+ messages in thread
From: xiubli @ 2022-07-01 2:29 UTC (permalink / raw)
To: jlayton, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs, Xiubo Li
From: Xiubo Li <xiubli@redhat.com>
kernel: page:00000000c9746ff1 refcount:2 mapcount:0 mapping:00000000dc2785bb index:0x1 pfn:0x141afc
kernel: memcg:ffff88810f766000
kernel: aops:ceph_aops [ceph] ino:100000005e7 dentry name:"postgresql-Fri.log"
kernel: flags: 0x5ffc000000201c(uptodate|dirty|lru|private|node=0|zone=2|lastcpupid=0x7ff)
kernel: raw: 005ffc000000201c ffffea000a9eeb48 ffffea00060ade48 ffff888193ed8228
kernel: raw: 0000000000000001 ffff88810cc96500 00000002ffffffff ffff88810f766000
kernel: page dumped because: VM_BUG_ON_FOLIO(!folio_test_locked(folio))
kernel: ------------[ cut here ]------------
kernel: kernel BUG at mm/filemap.c:1559!
kernel: invalid opcode: 0000 [#1] PREEMPT SMP PTI
kernel: CPU: 4 PID: 131697 Comm: postmaster Tainted: G S 5.19.0-rc2-ceph-g822a4c74e05d #1
kernel: Hardware name: Supermicro SYS-5018R-WR/X10SRW-F, BIOS 2.0 12/17/2015
kernel: RIP: 0010:folio_unlock+0x26/0x30
kernel: Code: 00 0f 1f 00 0f 1f 44 00 00 48 8b 07 a8 01 74 0e f0 80 27 fe 78 01 c3 31 f6 e9 d6 fe ff ff 48 c7 c6 c0 81 37 82 e8 aa 64 04 00 <0f> 0b 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 8b 87 b8 01 00 00
kernel: RSP: 0018:ffffc90004377bc8 EFLAGS: 00010246
kernel: RAX: 000000000000003f RBX: ffff888193ed8228 RCX: 0000000000000001
kernel: RDX: 0000000000000000 RSI: ffffffff823a3569 RDI: 00000000ffffffff
kernel: RBP: ffffffff828a0058 R08: 0000000000000001 R09: 0000000000000001
kernel: R10: 000000007c6b0fd2 R11: 0000000000000034 R12: 0000000000000001
kernel: R13: 00000000fffffe00 R14: ffffea000506bf00 R15: ffff888193ed8000
kernel: FS: 00007f4993626340(0000) GS:ffff88885fd00000(0000) knlGS:0000000000000000
kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 0000555789ee8000 CR3: 000000017a52a006 CR4: 00000000001706e0
kernel: Call Trace:
kernel: <TASK>
kernel: netfs_write_begin+0x130/0x950 [netfs]
kernel: ceph_write_begin+0x46/0xd0 [ceph]
kernel: generic_perform_write+0xef/0x200
kernel: ? file_update_time+0xd4/0x110
kernel: ceph_write_iter+0xb01/0xcd0 [ceph]
kernel: ? lock_is_held_type+0xe3/0x140
kernel: ? new_sync_write+0x106/0x180
kernel: new_sync_write+0x106/0x180
kernel: vfs_write+0x29a/0x3a0
kernel: ksys_write+0x5c/0xd0
kernel: do_syscall_64+0x34/0x80
kernel: entry_SYSCALL_64_after_hwframe+0x46/0xb0
kernel: RIP: 0033:0x7f49903205c8
kernel: Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 d5 3f 2a 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
kernel: RSP: 002b:00007fff104bd178 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
kernel: RAX: ffffffffffffffda RBX: 0000000000000048 RCX: 00007f49903205c8
kernel: RDX: 0000000000000048 RSI: 000055944d3c1ea0 RDI: 000000000000000b
kernel: RBP: 000055944d3c1ea0 R08: 000055944d3963d0 R09: 00007fff1055b080
kernel: R10: 0000000000000000 R11: 0000000000000246 R12: 000055944d3962f0
kernel: R13: 0000000000000048 R14: 00007f49905bb880 R15: 0000000000000048
kernel: </TASK>
Xiubo Li (2):
netfs: release the folio lock and put the folio before retrying
ceph: do not release the folio lock in kceph
fs/ceph/addr.c | 6 +++---
fs/netfs/buffered_read.c | 5 ++++-
2 files changed, 7 insertions(+), 4 deletions(-)
--
2.36.0.rc1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-01 2:29 [PATCH 0/2] netfs, ceph: fix the crash when unlocking the folio xiubli
@ 2022-07-01 2:29 ` xiubli
2022-07-01 10:38 ` Jeff Layton
2022-07-05 13:21 ` David Howells
2022-07-01 2:29 ` [PATCH 2/2] ceph: do not release the folio lock in kceph xiubli
1 sibling, 2 replies; 11+ messages in thread
From: xiubli @ 2022-07-01 2:29 UTC (permalink / raw)
To: jlayton, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs, Xiubo Li
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;
}
}
--
2.36.0.rc1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] ceph: do not release the folio lock in kceph
2022-07-01 2:29 [PATCH 0/2] netfs, ceph: fix the crash when unlocking the folio xiubli
2022-07-01 2:29 ` [PATCH 1/2] netfs: release the folio lock and put the folio before retrying xiubli
@ 2022-07-01 2:29 ` xiubli
1 sibling, 0 replies; 11+ messages in thread
From: xiubli @ 2022-07-01 2:29 UTC (permalink / raw)
To: jlayton, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs, Xiubo Li
From: Xiubo Li <xiubli@redhat.com>
The netfs layer should be responsible to unlock and put the folio,
and we will always return 0 when succeeds.
URL: https://tracker.ceph.com/issues/56423
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
fs/ceph/addr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index fe6147f20dee..3ef5200e2005 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1310,16 +1310,16 @@ static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned
if (snapc) {
int r;
- folio_unlock(folio);
- folio_put(folio);
if (IS_ERR(snapc))
return PTR_ERR(snapc);
+ folio_unlock(folio);
ceph_queue_writeback(inode);
r = wait_event_killable(ci->i_cap_wq,
context_is_writeable_or_written(inode, snapc));
ceph_put_snap_context(snapc);
- return r == 0 ? -EAGAIN : r;
+ folio_lock(folio);
+ return r == 0 ? -EAGAIN : 0;
}
return 0;
}
--
2.36.0.rc1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying
2022-07-01 2:29 ` [PATCH 1/2] netfs: release the folio lock and put the folio before retrying xiubli
@ 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
1 sibling, 2 replies; 11+ messages in thread
From: Jeff Layton @ 2022-07-01 10:38 UTC (permalink / raw)
To: xiubli, idryomov, dhowells
Cc: vshankar, linux-kernel, ceph-devel, willy, keescook,
linux-fsdevel, linux-cachefs
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;
}
^ permalink raw reply related [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 2:10 ` Matthew Wilcox
2022-07-04 6:58 ` Xiubo Li
1 sibling, 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
1 sibling, 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 2:29 ` [PATCH 1/2] netfs: release the folio lock and put the folio before retrying xiubli
2022-07-01 10:38 ` Jeff Layton
@ 2022-07-05 13:21 ` David Howells
2022-07-05 13:41 ` Jeff Layton
2022-07-06 0:58 ` Xiubo Li
1 sibling, 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
end of thread, other threads:[~2022-07-06 0:58 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-01 2:29 [PATCH 0/2] netfs, ceph: fix the crash when unlocking the folio xiubli
2022-07-01 2:29 ` [PATCH 1/2] netfs: release the folio lock and put the folio before retrying xiubli
2022-07-01 10:38 ` Jeff Layton
2022-07-04 1:13 ` Xiubo Li
2022-07-04 2:10 ` Matthew Wilcox
2022-07-04 2:40 ` 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
2022-07-01 2:29 ` [PATCH 2/2] ceph: do not release the folio lock in kceph xiubli
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).