Re: [PATCH v2] fs: create kiocb_{start,end}_write() helpers

[Jens Axboe <axboe@kernel.dk>]

On 8/16/23 2:54 AM, 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 and use the helpers in all the
> callers.
> 
> Use a new iocb flag IOCB_WRITE_STARTED to indicate if sb_start_write()
> was called.

Looks better now, but I think you should split this into a prep patch
that adds the helpers, and then one for each conversion. We've had bugs
with this accounting before which causes fs freeze issues, would be
prudent to have it split because of that.

> diff --git a/fs/aio.c b/fs/aio.c
> index 77e33619de40..16fb3ac2093b 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1444,17 +1444,8 @@ static void aio_complete_rw(struct kiocb *kiocb, long res)
>  	if (!list_empty_careful(&iocb->ki_list))
>  		aio_remove_iocb(iocb);
>  
> -	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);
> -	}
> +	if (kiocb->ki_flags & IOCB_WRITE)
> +		kiocb_end_write(kiocb);

Can't we just call kiocb_end_write() here, it checks WRITE_STARTED
anyway? Not a big deal, and honestly I'd rather just get rid of
WRITE_STARTED if we're not using it like that. It doesn't serve much of
a purpose, if we're gating this one IOCB_WRITE anyway (which I do like
better than WRITE_STARTED). And it avoids writing to the kiocb at the
end too, which is a nice win.

> index b2adee67f9b2..8e5d410a1be5 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -338,6 +338,8 @@ enum rw_hint {
>  #define IOCB_NOIO		(1 << 20)
>  /* can use bio alloc cache */
>  #define IOCB_ALLOC_CACHE	(1 << 21)
> +/* file_start_write() was called */
> +#define IOCB_WRITE_STARTED	(1 << 22)
>  
>  /* for use in trace events */
>  #define TRACE_IOCB_STRINGS \
> @@ -351,7 +353,8 @@ enum rw_hint {
>  	{ IOCB_WRITE,		"WRITE" }, \
>  	{ IOCB_WAITQ,		"WAITQ" }, \
>  	{ IOCB_NOIO,		"NOIO" }, \
> -	{ IOCB_ALLOC_CACHE,	"ALLOC_CACHE" }
> +	{ IOCB_ALLOC_CACHE,	"ALLOC_CACHE" }, \
> +	{ IOCB_WRITE_STARTED,	"WRITE_STARTED" }
>  
>  struct kiocb {
>  	struct file		*ki_filp;

These changes will conflict with other changes in linux-next that are
going upstream. I'd prefer to stage this one after those changes, once
we get to a version that looks good to everybody.

-- 
Jens Axboe