Re: [PATCH 1/2] netfs: release the folio lock and put the folio before retrying

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 7/1/22 6:38 PM, Jeff Layton wrote:
On Fri, 2022-07-01 at 10:29 +0800, xiubli@xxxxxxxxxx wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>

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@xxxxxxxxxx>
---
  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;
  }





[Index of Archives]     [CEPH Users]     [Ceph Large]     [Ceph Dev]     [Information on CEPH]     [Linux BTRFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux