[PATCH v3 0/7] kiocb_{start,end}_write() helpers

[PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Amir Goldstein]

Christian,

This is an attempt to consolidate the open coded lockdep fooling in
all those async io submitters into a single helper.
The idea to do that consolidation was suggested by Jan.

This re-factoring is part of a larger vfs cleanup I am doing for
fanotify permission events.  The complete series is not ready for
prime time yet, but this one patch is independent and I would love
to get it reviewed/merged a head of the rest.

This v3 series addresses the review comments of Jens on v2 [1].

Thanks,
Amir.

Changes since v2:
- Split patches to helpers and use helpers per caller (Jens)
- Drop IOCB_WRITE_STARTED flag

Changes since v1:
- Leave ISREG checks in callers (Jens)
- Leave setting IOCB_WRITE flag in callers

[1] https://lore.kernel.org/linux-fsdevel/20230816085439.894112-1-amir73il@gmail.com/

Amir Goldstein (7):
  io_uring: rename kiocb_end_write() local helper
  fs: add kerneldoc to file_{start,end}_write() helpers
  fs: create kiocb_{start,end}_write() helpers
  io_uring: use kiocb_{start,end}_write() helpers
  aio: use kiocb_{start,end}_write() helpers
  ovl: use kiocb_{start,end}_write() helpers
  cachefiles: use kiocb_{start,end}_write() helpers

 fs/aio.c            | 20 +++---------------
 fs/cachefiles/io.c  | 16 +++-----------
 fs/overlayfs/file.c | 10 ++-------
 include/linux/fs.h  | 51 ++++++++++++++++++++++++++++++++++++++++++++-
 io_uring/rw.c       | 33 ++++++++---------------------
 5 files changed, 67 insertions(+), 63 deletions(-)

-- 
2.34.1

[PATCH v3 1/7] io_uring: rename kiocb_end_write() local helper

[Amir Goldstein]

This helper does not take a kiocb as input and we want to create a
common helper by that name that takes a kiocb as input.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 io_uring/rw.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 1bce2208b65c..749ebd565839 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -220,7 +220,7 @@ static bool io_rw_should_reissue(struct io_kiocb *req)
 }
 #endif
 
-static void kiocb_end_write(struct io_kiocb *req)
+static void io_req_end_write(struct io_kiocb *req)
 {
 	/*
 	 * Tell lockdep we inherited freeze protection from submission
@@ -243,7 +243,7 @@ static void io_req_io_end(struct io_kiocb *req)
 	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
 
 	if (rw->kiocb.ki_flags & IOCB_WRITE) {
-		kiocb_end_write(req);
+		io_req_end_write(req);
 		fsnotify_modify(req->file);
 	} else {
 		fsnotify_access(req->file);
@@ -313,7 +313,7 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
 	struct io_kiocb *req = cmd_to_io_kiocb(rw);
 
 	if (kiocb->ki_flags & IOCB_WRITE)
-		kiocb_end_write(req);
+		io_req_end_write(req);
 	if (unlikely(res != req->cqe.res)) {
 		if (res == -EAGAIN && io_rw_should_reissue(req)) {
 			req->flags |= REQ_F_REISSUE | REQ_F_PARTIAL_IO;
@@ -961,7 +961,7 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
 				io->bytes_done += ret2;
 
 			if (kiocb->ki_flags & IOCB_WRITE)
-				kiocb_end_write(req);
+				io_req_end_write(req);
 			return ret ? ret : -EAGAIN;
 		}
 done:
@@ -972,7 +972,7 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		ret = io_setup_async_rw(req, iovec, s, false);
 		if (!ret) {
 			if (kiocb->ki_flags & IOCB_WRITE)
-				kiocb_end_write(req);
+				io_req_end_write(req);
 			return -EAGAIN;
 		}
 		return ret;
-- 
2.34.1

Re: [PATCH v3 1/7] io_uring: rename kiocb_end_write() local helper

[Jan Kara]

On Thu 17-08-23 17:13:31, Amir Goldstein wrote:
> This helper does not take a kiocb as input and we want to create a
> common helper by that name that takes a kiocb as input.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  io_uring/rw.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 1bce2208b65c..749ebd565839 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -220,7 +220,7 @@ static bool io_rw_should_reissue(struct io_kiocb *req)
>  }
>  #endif
>  
> -static void kiocb_end_write(struct io_kiocb *req)
> +static void io_req_end_write(struct io_kiocb *req)
>  {
>  	/*
>  	 * Tell lockdep we inherited freeze protection from submission
> @@ -243,7 +243,7 @@ static void io_req_io_end(struct io_kiocb *req)
>  	struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
>  
>  	if (rw->kiocb.ki_flags & IOCB_WRITE) {
> -		kiocb_end_write(req);
> +		io_req_end_write(req);
>  		fsnotify_modify(req->file);
>  	} else {
>  		fsnotify_access(req->file);
> @@ -313,7 +313,7 @@ static void io_complete_rw_iopoll(struct kiocb *kiocb, long res)
>  	struct io_kiocb *req = cmd_to_io_kiocb(rw);
>  
>  	if (kiocb->ki_flags & IOCB_WRITE)
> -		kiocb_end_write(req);
> +		io_req_end_write(req);
>  	if (unlikely(res != req->cqe.res)) {
>  		if (res == -EAGAIN && io_rw_should_reissue(req)) {
>  			req->flags |= REQ_F_REISSUE | REQ_F_PARTIAL_IO;
> @@ -961,7 +961,7 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
>  				io->bytes_done += ret2;
>  
>  			if (kiocb->ki_flags & IOCB_WRITE)
> -				kiocb_end_write(req);
> +				io_req_end_write(req);
>  			return ret ? ret : -EAGAIN;
>  		}
>  done:
> @@ -972,7 +972,7 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
>  		ret = io_setup_async_rw(req, iovec, s, false);
>  		if (!ret) {
>  			if (kiocb->ki_flags & IOCB_WRITE)
> -				kiocb_end_write(req);
> +				io_req_end_write(req);
>  			return -EAGAIN;
>  		}
>  		return ret;
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 2/7] fs: add kerneldoc to file_{start,end}_write() helpers

[Amir Goldstein]

and use sb_end_write() instead of open coded version.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index b2adee67f9b2..ced388aff51f 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2545,6 +2545,13 @@ static inline bool inode_wrong_type(const struct inode *inode, umode_t mode)
 	return (inode->i_mode ^ mode) & S_IFMT;
 }
 
+/**
+ * file_start_write - get write access to a superblock for regular file io
+ * @file: the file we want to write to
+ *
+ * This is a variant of sb_start_write() which is a noop on non-regualr file.
+ * Should be matched with a call to file_end_write().
+ */
 static inline void file_start_write(struct file *file)
 {
 	if (!S_ISREG(file_inode(file)->i_mode))
@@ -2559,11 +2566,17 @@ static inline bool file_start_write_trylock(struct file *file)
 	return sb_start_write_trylock(file_inode(file)->i_sb);
 }
 
+/**
+ * file_end_write - drop write access to a superblock of a regular file
+ * @file: the file we wrote to
+ *
+ * Should be matched with a call to file_start_write().
+ */
 static inline void file_end_write(struct file *file)
 {
 	if (!S_ISREG(file_inode(file)->i_mode))
 		return;
-	__sb_end_write(file_inode(file)->i_sb, SB_FREEZE_WRITE);
+	sb_end_write(file_inode(file)->i_sb);
 }
 
 /*
-- 
2.34.1

Re: [PATCH v3 2/7] fs: add kerneldoc to file_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:32, Amir Goldstein wrote:
> and use sb_end_write() instead of open coded version.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/fs.h | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b2adee67f9b2..ced388aff51f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2545,6 +2545,13 @@ static inline bool inode_wrong_type(const struct inode *inode, umode_t mode)
>  	return (inode->i_mode ^ mode) & S_IFMT;
>  }
>  
> +/**
> + * file_start_write - get write access to a superblock for regular file io
> + * @file: the file we want to write to
> + *
> + * This is a variant of sb_start_write() which is a noop on non-regualr file.
> + * Should be matched with a call to file_end_write().
> + */
>  static inline void file_start_write(struct file *file)
>  {
>  	if (!S_ISREG(file_inode(file)->i_mode))
> @@ -2559,11 +2566,17 @@ static inline bool file_start_write_trylock(struct file *file)
>  	return sb_start_write_trylock(file_inode(file)->i_sb);
>  }
>  
> +/**
> + * file_end_write - drop write access to a superblock of a regular file
> + * @file: the file we wrote to
> + *
> + * Should be matched with a call to file_start_write().
> + */
>  static inline void file_end_write(struct file *file)
>  {
>  	if (!S_ISREG(file_inode(file)->i_mode))
>  		return;
> -	__sb_end_write(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> +	sb_end_write(file_inode(file)->i_sb);
>  }
>  
>  /*
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 3/7] fs: create kiocb_{start,end}_write() helpers

[Amir Goldstein]

aio, io_uring, cachefiles and overlayfs, all open code an ugly variant
of file_{start,end}_write() to silence lockdep warnings.

Create helpers for this lockdep dance so we can use the helpers in all
the callers.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fs.h | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/include/linux/fs.h b/include/linux/fs.h
index ced388aff51f..2548048a6e6c 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2579,6 +2579,42 @@ static inline void file_end_write(struct file *file)
 	sb_end_write(file_inode(file)->i_sb);
 }
 
+/**
+ * kiocb_start_write - get write access to a superblock for async file io
+ * @iocb: the io context we want to submit the write with
+ *
+ * This is a variant of sb_start_write() for async io submission.
+ * Should be matched with a call to kiocb_end_write().
+ */
+static inline void kiocb_start_write(struct kiocb *iocb)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	sb_start_write(inode->i_sb);
+	/*
+	 * Fool lockdep by telling it the lock got released so that it
+	 * doesn't complain about the held lock when we return to userspace.
+	 */
+	__sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
+}
+
+/**
+ * kiocb_end_write - drop write access to a superblock after async file io
+ * @iocb: the io context we sumbitted the write with
+ *
+ * Should be matched with a call to kiocb_start_write().
+ */
+static inline void kiocb_end_write(struct kiocb *iocb)
+{
+	struct inode *inode = file_inode(iocb->ki_filp);
+
+	/*
+	 * Tell lockdep we inherited freeze protection from submission thread.
+	 */
+	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
+	sb_end_write(inode->i_sb);
+}
+
 /*
  * This is used for regular files where some users -- especially the
  * currently executed binary in a process, previously handled via
-- 
2.34.1

Re: [PATCH v3 3/7] fs: create kiocb_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:33, Amir Goldstein wrote:
> aio, io_uring, cachefiles and overlayfs, all open code an ugly variant
> of file_{start,end}_write() to silence lockdep warnings.
> 
> Create helpers for this lockdep dance so we can use the helpers in all
> the callers.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  include/linux/fs.h | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index ced388aff51f..2548048a6e6c 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2579,6 +2579,42 @@ static inline void file_end_write(struct file *file)
>  	sb_end_write(file_inode(file)->i_sb);
>  }
>  
> +/**
> + * kiocb_start_write - get write access to a superblock for async file io
> + * @iocb: the io context we want to submit the write with
> + *
> + * This is a variant of sb_start_write() for async io submission.
> + * Should be matched with a call to kiocb_end_write().
> + */
> +static inline void kiocb_start_write(struct kiocb *iocb)
> +{
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	sb_start_write(inode->i_sb);
> +	/*
> +	 * Fool lockdep by telling it the lock got released so that it
> +	 * doesn't complain about the held lock when we return to userspace.
> +	 */
> +	__sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
> +}
> +
> +/**
> + * kiocb_end_write - drop write access to a superblock after async file io
> + * @iocb: the io context we sumbitted the write with
> + *
> + * Should be matched with a call to kiocb_start_write().
> + */
> +static inline void kiocb_end_write(struct kiocb *iocb)
> +{
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +
> +	/*
> +	 * Tell lockdep we inherited freeze protection from submission thread.
> +	 */
> +	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
> +	sb_end_write(inode->i_sb);
> +}
> +
>  /*
>   * This is used for regular files where some users -- especially the
>   * currently executed binary in a process, previously handled via
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 4/7] io_uring: use kiocb_{start,end}_write() helpers

[Amir Goldstein]

Use helpers instead of the open coded dance to silence lockdep warnings.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 io_uring/rw.c | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

diff --git a/io_uring/rw.c b/io_uring/rw.c
index 749ebd565839..9581b90cb459 100644
--- a/io_uring/rw.c
+++ b/io_uring/rw.c
@@ -222,15 +222,10 @@ static bool io_rw_should_reissue(struct io_kiocb *req)
 
 static void io_req_end_write(struct io_kiocb *req)
 {
-	/*
-	 * Tell lockdep we inherited freeze protection from submission
-	 * thread.
-	 */
 	if (req->flags & REQ_F_ISREG) {
-		struct super_block *sb = file_inode(req->file)->i_sb;
+		struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
 
-		__sb_writers_acquired(sb, SB_FREEZE_WRITE);
-		sb_end_write(sb);
+		kiocb_end_write(&rw->kiocb);
 	}
 }
 
@@ -902,18 +897,8 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
 		return ret;
 	}
 
-	/*
-	 * Open-code file_start_write here to grab freeze protection,
-	 * which will be released by another thread in
-	 * io_complete_rw().  Fool lockdep by telling it the lock got
-	 * released so that it doesn't complain about the held lock when
-	 * we return to userspace.
-	 */
-	if (req->flags & REQ_F_ISREG) {
-		sb_start_write(file_inode(req->file)->i_sb);
-		__sb_writers_release(file_inode(req->file)->i_sb,
-					SB_FREEZE_WRITE);
-	}
+	if (req->flags & REQ_F_ISREG)
+		kiocb_start_write(kiocb);
 	kiocb->ki_flags |= IOCB_WRITE;
 
 	if (likely(req->file->f_op->write_iter))
-- 
2.34.1

Re: [PATCH v3 4/7] io_uring: use kiocb_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:34, Amir Goldstein wrote:
> Use helpers instead of the open coded dance to silence lockdep warnings.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  io_uring/rw.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
> 
> diff --git a/io_uring/rw.c b/io_uring/rw.c
> index 749ebd565839..9581b90cb459 100644
> --- a/io_uring/rw.c
> +++ b/io_uring/rw.c
> @@ -222,15 +222,10 @@ static bool io_rw_should_reissue(struct io_kiocb *req)
>  
>  static void io_req_end_write(struct io_kiocb *req)
>  {
> -	/*
> -	 * Tell lockdep we inherited freeze protection from submission
> -	 * thread.
> -	 */
>  	if (req->flags & REQ_F_ISREG) {
> -		struct super_block *sb = file_inode(req->file)->i_sb;
> +		struct io_rw *rw = io_kiocb_to_cmd(req, struct io_rw);
>  
> -		__sb_writers_acquired(sb, SB_FREEZE_WRITE);
> -		sb_end_write(sb);
> +		kiocb_end_write(&rw->kiocb);
>  	}
>  }
>  
> @@ -902,18 +897,8 @@ int io_write(struct io_kiocb *req, unsigned int issue_flags)
>  		return ret;
>  	}
>  
> -	/*
> -	 * Open-code file_start_write here to grab freeze protection,
> -	 * which will be released by another thread in
> -	 * io_complete_rw().  Fool lockdep by telling it the lock got
> -	 * released so that it doesn't complain about the held lock when
> -	 * we return to userspace.
> -	 */
> -	if (req->flags & REQ_F_ISREG) {
> -		sb_start_write(file_inode(req->file)->i_sb);
> -		__sb_writers_release(file_inode(req->file)->i_sb,
> -					SB_FREEZE_WRITE);
> -	}
> +	if (req->flags & REQ_F_ISREG)
> +		kiocb_start_write(kiocb);
>  	kiocb->ki_flags |= IOCB_WRITE;
>  
>  	if (likely(req->file->f_op->write_iter))
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 5/7] aio: use kiocb_{start,end}_write() helpers

[Amir Goldstein]

Use helpers instead of the open coded dance to silence lockdep warnings.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/aio.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/fs/aio.c b/fs/aio.c
index 77e33619de40..b3174da80ff6 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -1447,13 +1447,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
 	if (kiocb->ki_flags & IOCB_WRITE) {
 		struct inode *inode = file_inode(kiocb->ki_filp);
 
-		/*
-		 * Tell lockdep we inherited freeze protection from submission
-		 * thread.
-		 */
 		if (S_ISREG(inode->i_mode))
-			__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
-		file_end_write(kiocb->ki_filp);
+			kiocb_end_write(kiocb);
 	}
 
 	iocb->ki_res.res = res;
@@ -1581,17 +1576,8 @@ static int aio_write(struct kiocb *req, const struct iocb *iocb,
 		return ret;
 	ret = rw_verify_area(WRITE, file, &req->ki_pos, iov_iter_count(&iter));
 	if (!ret) {
-		/*
-		 * Open-code file_start_write here to grab freeze protection,
-		 * which will be released by another thread in
-		 * aio_complete_rw().  Fool lockdep by telling it the lock got
-		 * released so that it doesn't complain about the held lock when
-		 * we return to userspace.
-		 */
-		if (S_ISREG(file_inode(file)->i_mode)) {
-			sb_start_write(file_inode(file)->i_sb);
-			__sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
-		}
+		if (S_ISREG(file_inode(file)->i_mode))
+			kiocb_start_write(req);
 		req->ki_flags |= IOCB_WRITE;
 		aio_rw_done(req, call_write_iter(file, req, &iter));
 	}
-- 
2.34.1

Re: [PATCH v3 5/7] aio: use kiocb_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:35, Amir Goldstein wrote:
> Use helpers instead of the open coded dance to silence lockdep warnings.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/aio.c | 20 +++-----------------
>  1 file changed, 3 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 77e33619de40..b3174da80ff6 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1447,13 +1447,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
>  	if (kiocb->ki_flags & IOCB_WRITE) {
>  		struct inode *inode = file_inode(kiocb->ki_filp);
>  
> -		/*
> -		 * Tell lockdep we inherited freeze protection from submission
> -		 * thread.
> -		 */
>  		if (S_ISREG(inode->i_mode))
> -			__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
> -		file_end_write(kiocb->ki_filp);
> +			kiocb_end_write(kiocb);
>  	}
>  
>  	iocb->ki_res.res = res;
> @@ -1581,17 +1576,8 @@ static int aio_write(struct kiocb *req, const struct iocb *iocb,
>  		return ret;
>  	ret = rw_verify_area(WRITE, file, &req->ki_pos, iov_iter_count(&iter));
>  	if (!ret) {
> -		/*
> -		 * Open-code file_start_write here to grab freeze protection,
> -		 * which will be released by another thread in
> -		 * aio_complete_rw().  Fool lockdep by telling it the lock got
> -		 * released so that it doesn't complain about the held lock when
> -		 * we return to userspace.
> -		 */
> -		if (S_ISREG(file_inode(file)->i_mode)) {
> -			sb_start_write(file_inode(file)->i_sb);
> -			__sb_writers_release(file_inode(file)->i_sb, SB_FREEZE_WRITE);
> -		}
> +		if (S_ISREG(file_inode(file)->i_mode))
> +			kiocb_start_write(req);
>  		req->ki_flags |= IOCB_WRITE;
>  		aio_rw_done(req, call_write_iter(file, req, &iter));
>  	}
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 6/7] ovl: use kiocb_{start,end}_write() helpers

[Amir Goldstein]

Use helpers instead of the open coded dance to silence lockdep warnings.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/file.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 21245b00722a..e4cc401d334d 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -290,10 +290,7 @@ static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
 	if (iocb->ki_flags & IOCB_WRITE) {
 		struct inode *inode = file_inode(orig_iocb->ki_filp);
 
-		/* Actually acquired in ovl_write_iter() */
-		__sb_writers_acquired(file_inode(iocb->ki_filp)->i_sb,
-				      SB_FREEZE_WRITE);
-		file_end_write(iocb->ki_filp);
+		kiocb_end_write(iocb);
 		ovl_copyattr(inode);
 	}
 
@@ -409,10 +406,6 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 		if (!aio_req)
 			goto out;
 
-		file_start_write(real.file);
-		/* Pacify lockdep, same trick as done in aio_write() */
-		__sb_writers_release(file_inode(real.file)->i_sb,
-				     SB_FREEZE_WRITE);
 		aio_req->fd = real;
 		real.flags = 0;
 		aio_req->orig_iocb = iocb;
@@ -420,6 +413,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 		aio_req->iocb.ki_flags = ifl;
 		aio_req->iocb.ki_complete = ovl_aio_rw_complete;
 		refcount_set(&aio_req->ref, 2);
+		kiocb_start_write(&aio_req->iocb);
 		ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter);
 		ovl_aio_put(aio_req);
 		if (ret != -EIOCBQUEUED)
-- 
2.34.1

Re: [PATCH v3 6/7] ovl: use kiocb_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:36, Amir Goldstein wrote:
> Use helpers instead of the open coded dance to silence lockdep warnings.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/overlayfs/file.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 21245b00722a..e4cc401d334d 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -290,10 +290,7 @@ static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
>  	if (iocb->ki_flags & IOCB_WRITE) {
>  		struct inode *inode = file_inode(orig_iocb->ki_filp);
>  
> -		/* Actually acquired in ovl_write_iter() */
> -		__sb_writers_acquired(file_inode(iocb->ki_filp)->i_sb,
> -				      SB_FREEZE_WRITE);
> -		file_end_write(iocb->ki_filp);
> +		kiocb_end_write(iocb);
>  		ovl_copyattr(inode);
>  	}
>  
> @@ -409,10 +406,6 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		if (!aio_req)
>  			goto out;
>  
> -		file_start_write(real.file);
> -		/* Pacify lockdep, same trick as done in aio_write() */
> -		__sb_writers_release(file_inode(real.file)->i_sb,
> -				     SB_FREEZE_WRITE);
>  		aio_req->fd = real;
>  		real.flags = 0;
>  		aio_req->orig_iocb = iocb;
> @@ -420,6 +413,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		aio_req->iocb.ki_flags = ifl;
>  		aio_req->iocb.ki_complete = ovl_aio_rw_complete;
>  		refcount_set(&aio_req->ref, 2);
> +		kiocb_start_write(&aio_req->iocb);
>  		ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter);
>  		ovl_aio_put(aio_req);
>  		if (ret != -EIOCBQUEUED)
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v3 7/7] cachefiles: use kiocb_{start,end}_write() helpers

[Amir Goldstein]

Use helpers instead of the open coded dance to silence lockdep warnings.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/cachefiles/io.c | 16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
index 175a25fcade8..009d23cd435b 100644
--- a/fs/cachefiles/io.c
+++ b/fs/cachefiles/io.c
@@ -259,9 +259,7 @@ static void cachefiles_write_complete(struct kiocb *iocb, long ret)
 
 	_enter("%ld", ret);
 
-	/* Tell lockdep we inherited freeze protection from submission thread */
-	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
-	__sb_end_write(inode->i_sb, SB_FREEZE_WRITE);
+	kiocb_end_write(iocb);
 
 	if (ret < 0)
 		trace_cachefiles_io_error(object, inode, ret,
@@ -286,7 +284,6 @@ int __cachefiles_write(struct cachefiles_object *object,
 {
 	struct cachefiles_cache *cache;
 	struct cachefiles_kiocb *ki;
-	struct inode *inode;
 	unsigned int old_nofs;
 	ssize_t ret;
 	size_t len = iov_iter_count(iter);
@@ -322,19 +319,12 @@ int __cachefiles_write(struct cachefiles_object *object,
 		ki->iocb.ki_complete = cachefiles_write_complete;
 	atomic_long_add(ki->b_writing, &cache->b_writing);
 
-	/* Open-code file_start_write here to grab freeze protection, which
-	 * will be released by another thread in aio_complete_rw().  Fool
-	 * lockdep by telling it the lock got released so that it doesn't
-	 * complain about the held lock when we return to userspace.
-	 */
-	inode = file_inode(file);
-	__sb_start_write(inode->i_sb, SB_FREEZE_WRITE);
-	__sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
+	kiocb_start_write(&ki->iocb);
 
 	get_file(ki->iocb.ki_filp);
 	cachefiles_grab_object(object, cachefiles_obj_get_ioreq);
 
-	trace_cachefiles_write(object, inode, ki->iocb.ki_pos, len);
+	trace_cachefiles_write(object, file_inode(file), ki->iocb.ki_pos, len);
 	old_nofs = memalloc_nofs_save();
 	ret = cachefiles_inject_write_error();
 	if (ret == 0)
-- 
2.34.1

Re: [PATCH v3 7/7] cachefiles: use kiocb_{start,end}_write() helpers

[Jan Kara]

On Thu 17-08-23 17:13:37, Amir Goldstein wrote:
> Use helpers instead of the open coded dance to silence lockdep warnings.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/cachefiles/io.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/cachefiles/io.c b/fs/cachefiles/io.c
> index 175a25fcade8..009d23cd435b 100644
> --- a/fs/cachefiles/io.c
> +++ b/fs/cachefiles/io.c
> @@ -259,9 +259,7 @@ static void cachefiles_write_complete(struct kiocb *iocb, long ret)
>  
>  	_enter("%ld", ret);
>  
> -	/* Tell lockdep we inherited freeze protection from submission thread */
> -	__sb_writers_acquired(inode->i_sb, SB_FREEZE_WRITE);
> -	__sb_end_write(inode->i_sb, SB_FREEZE_WRITE);
> +	kiocb_end_write(iocb);
>  
>  	if (ret < 0)
>  		trace_cachefiles_io_error(object, inode, ret,
> @@ -286,7 +284,6 @@ int __cachefiles_write(struct cachefiles_object *object,
>  {
>  	struct cachefiles_cache *cache;
>  	struct cachefiles_kiocb *ki;
> -	struct inode *inode;
>  	unsigned int old_nofs;
>  	ssize_t ret;
>  	size_t len = iov_iter_count(iter);
> @@ -322,19 +319,12 @@ int __cachefiles_write(struct cachefiles_object *object,
>  		ki->iocb.ki_complete = cachefiles_write_complete;
>  	atomic_long_add(ki->b_writing, &cache->b_writing);
>  
> -	/* Open-code file_start_write here to grab freeze protection, which
> -	 * will be released by another thread in aio_complete_rw().  Fool
> -	 * lockdep by telling it the lock got released so that it doesn't
> -	 * complain about the held lock when we return to userspace.
> -	 */
> -	inode = file_inode(file);
> -	__sb_start_write(inode->i_sb, SB_FREEZE_WRITE);
> -	__sb_writers_release(inode->i_sb, SB_FREEZE_WRITE);
> +	kiocb_start_write(&ki->iocb);
>  
>  	get_file(ki->iocb.ki_filp);
>  	cachefiles_grab_object(object, cachefiles_obj_get_ioreq);
>  
> -	trace_cachefiles_write(object, inode, ki->iocb.ki_pos, len);
> +	trace_cachefiles_write(object, file_inode(file), ki->iocb.ki_pos, len);
>  	old_nofs = memalloc_nofs_save();
>  	ret = cachefiles_inject_write_error();
>  	if (ret == 0)
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Christian Brauner]

On Thu, Aug 17, 2023 at 05:13:30PM +0300, Amir Goldstein wrote:
> Christian,
> 
> This is an attempt to consolidate the open coded lockdep fooling in
> all those async io submitters into a single helper.
> The idea to do that consolidation was suggested by Jan.
> 
> This re-factoring is part of a larger vfs cleanup I am doing for
> fanotify permission events.  The complete series is not ready for
> prime time yet, but this one patch is independent and I would love
> to get it reviewed/merged a head of the rest.
> 
> This v3 series addresses the review comments of Jens on v2 [1].

I have neither quarrels nor strong opinions on this so if Jens tells me
it looks fine to him I can take it.

Re: [PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Amir Goldstein]

On Thu, Aug 17, 2023 at 5:56 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Aug 17, 2023 at 05:13:30PM +0300, Amir Goldstein wrote:
> > Christian,
> >
> > This is an attempt to consolidate the open coded lockdep fooling in
> > all those async io submitters into a single helper.
> > The idea to do that consolidation was suggested by Jan.
> >
> > This re-factoring is part of a larger vfs cleanup I am doing for
> > fanotify permission events.  The complete series is not ready for
> > prime time yet, but this one patch is independent and I would love
> > to get it reviewed/merged a head of the rest.
> >
> > This v3 series addresses the review comments of Jens on v2 [1].
>
> I have neither quarrels nor strong opinions on this so if Jens tells me
> it looks fine to him I can take it.

That would be great.

Jens, do you approve of v3?

Jan, I see that you acked all patches except for 4/7 - I assume this
was an oversight?

Thanks,
Amir.

Re: [PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Jan Kara]

On Mon 21-08-23 14:35:14, Amir Goldstein wrote:
> On Thu, Aug 17, 2023 at 5:56 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Aug 17, 2023 at 05:13:30PM +0300, Amir Goldstein wrote:
> > > Christian,
> > >
> > > This is an attempt to consolidate the open coded lockdep fooling in
> > > all those async io submitters into a single helper.
> > > The idea to do that consolidation was suggested by Jan.
> > >
> > > This re-factoring is part of a larger vfs cleanup I am doing for
> > > fanotify permission events.  The complete series is not ready for
> > > prime time yet, but this one patch is independent and I would love
> > > to get it reviewed/merged a head of the rest.
> > >
> > > This v3 series addresses the review comments of Jens on v2 [1].
> >
> > I have neither quarrels nor strong opinions on this so if Jens tells me
> > it looks fine to him I can take it.
> 
> That would be great.
> 
> Jens, do you approve of v3?
> 
> Jan, I see that you acked all patches except for 4/7 - I assume this
> was an oversight?

Yes, posted now.
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Christian Brauner]

On Thu, 17 Aug 2023 17:13:30 +0300, Amir Goldstein wrote:
> Christian,
> 
> This is an attempt to consolidate the open coded lockdep fooling in
> all those async io submitters into a single helper.
> The idea to do that consolidation was suggested by Jan.
> 
> This re-factoring is part of a larger vfs cleanup I am doing for
> fanotify permission events.  The complete series is not ready for
> prime time yet, but this one patch is independent and I would love
> to get it reviewed/merged a head of the rest.
> 
> [...]

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/7] io_uring: rename kiocb_end_write() local helper
      https://git.kernel.org/vfs/vfs/c/790c4d42a7c0
[2/7] fs: add kerneldoc to file_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/7e6aa5e65f14
[3/7] fs: create kiocb_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/e15f778bb179
[4/7] io_uring: use kiocb_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/a2aafea26bd5
[5/7] aio: use kiocb_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/b91bf89f2435
[6/7] ovl: use kiocb_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/6c2366036a05
[7/7] cachefiles: use kiocb_{start,end}_write() helpers
      https://git.kernel.org/vfs/vfs/c/e7905aa6226c

Re: [PATCH v3 0/7] kiocb_{start,end}_write() helpers

[Jens Axboe]

On 8/17/23 8:13 AM, Amir Goldstein wrote:
> Christian,
> 
> This is an attempt to consolidate the open coded lockdep fooling in
> all those async io submitters into a single helper.
> The idea to do that consolidation was suggested by Jan.
> 
> This re-factoring is part of a larger vfs cleanup I am doing for
> fanotify permission events.  The complete series is not ready for
> prime time yet, but this one patch is independent and I would love
> to get it reviewed/merged a head of the rest.
> 
> This v3 series addresses the review comments of Jens on v2 [1].

Looks good to me now:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe