Messages in this thread | | | Subject | Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying | From | Xiubo Li <> | Date | Mon, 4 Jul 2022 14:58:33 +0800 |
| |
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; > } >
| |