Received-SPF: pass (google.com: domain of linux-ext4+bounces-1457-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Subject: Re: [PATCH RFC 1/2] iomap: Add a IOMAP_DIO_MAY_INLINE_COMP flag
To: Dave Chinner <david@fromorbit.com>
CC: <brauner@kernel.org>, <djwong@kernel.org>, <jack@suse.cz>,
	<tytso@mit.edu>, <linux-xfs@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<linux-ext4@vger.kernel.org>, <yi.zhang@huawei.com>
References: <20240229113849.2222577-1-chengzhihao1@huawei.com>
 <20240229113849.2222577-2-chengzhihao1@huawei.com>
 <ZeEkFUCUQ4eR7AlX@dread.disaster.area>
From: Zhihao Cheng <chengzhihao1@huawei.com>
Message-ID: <3de3ede5-31e0-2b7b-f523-9fd22090401f@huawei.com>
Date: Fri, 1 Mar 2024 18:02:10 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.5.0
Precedence: bulk
MIME-Version: 1.0
In-Reply-To: <ZeEkFUCUQ4eR7AlX@dread.disaster.area>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 8bit

?? 2024/3/1 8:40, Dave Chinner д??:

Hi Dave, thanks for your detailed and nice suggestions, I have a few 
questions below.
> On Thu, Feb 29, 2024 at 07:38:48PM +0800, Zhihao Cheng wrote:
>> It will be more efficient to execute quick endio process(eg. non-sync
>> overwriting case) under irq process rather than starting a worker to
>> do it.
>> Add a flag to control DIO to be finished inline(under irq context), which
>> can be used for non-sync overwriting case.
>> Besides, skip invalidating pages if DIO is finished inline, which will
>> keep the same logic with dio_bio_end_aio in non-sync overwriting case.
>>
>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
> 
> A nice idea, but I don't think an ext4 specific API flag is the
> right way to go about enabling this. The iomap dio code knows if
> the write is pure overwrite already - we have the IOMAP_F_DIRTY flag
> for that, and we combine this with IOMAP_DIO_WRITE_THROUGH to do the
> pure overwrite FUA optimisations.
> 
> That is:
> 
> 		/*
>                   * Use a FUA write if we need datasync semantics, this is a pure
>                   * data IO that doesn't require any metadata updates (including
>                   * after IO completion such as unwritten extent conversion) and
>                   * the underlying device either supports FUA or doesn't have
>                   * a volatile write cache. This allows us to avoid cache flushes
>                   * on IO completion. If we can't use writethrough and need to
>                   * sync, disable in-task completions as dio completion will
>                   * need to call generic_write_sync() which will do a blocking
>                   * fsync / cache flush call.
>                   */
>                  if (!(iomap->flags & (IOMAP_F_SHARED|IOMAP_F_DIRTY)) &&
>                      (dio->flags & IOMAP_DIO_WRITE_THROUGH) &&
>                      (bdev_fua(iomap->bdev) || !bdev_write_cache(iomap->bdev)))
>                          use_fua = true;
> 
> Hence if we want to optimise pure overwrites that have no data sync
> requirements, we already have the detection and triggers in place to
> do this. We just need to change the way we set up the IO flags to
> allow write-through (i.e. non-blocking IO completions) to use inline
> completions.
> 
> In __iomap_dio_rw():
> 
> +	/* Always try to complete inline. */
> +	dio->flags |= IOMAP_DIO_INLINE_COMP;
> 	if (iov_iter_rw(iter) == READ) {
> -               /* reads can always complete inline */
> -               dio->flags |= IOMAP_DIO_INLINE_COMP;
> ....
> 
> 	} else {
> +		/* Always try write-through semantics. If we can't
> +		 * use writethough, it will be disabled along with
> +		 * IOMAP_DIO_INLINE_COMP before dio completion is run
> +		 * so it can be deferred to a task completion context
> +		 * appropriately.
> +		 */
> +               dio->flags |= IOMAP_DIO_WRITE | IOMAP_DIO_WRITE_THROUGH;

There is a behavior change here, if we set IOMAP_DIO_WRITE_THROUGH 
unconditionally, non-datasync IO will be set with REQ_FUA, which means 
that device will flush writecache for each IO, will it affect the 
performance in non-sync dio case?
> 		iomi.flags |= IOMAP_WRITE;
> -               dio->flags |= IOMAP_DIO_WRITE;
> .....
> 		/* for data sync or sync, we need sync completion processing */
>                  if (iocb_is_dsync(iocb)) {
>                          dio->flags |= IOMAP_DIO_NEED_SYNC;
> 
> -                      /*
> -                       * For datasync only writes, we optimistically try using
> -                       * WRITE_THROUGH for this IO. This flag requires either
> -                       * FUA writes through the device's write cache, or a
> -                       * normal write to a device without a volatile write
> -                       * cache. For the former, Any non-FUA write that occurs
> -                       * will clear this flag, hence we know before completion
> -                       * whether a cache flush is necessary.
> -                       */
> -                       if (!(iocb->ki_flags & IOCB_SYNC))
> -                               dio->flags |= IOMAP_DIO_WRITE_THROUGH;
> +			* For sync writes we know we are going to need
> +			* blocking completion processing, so turn off
> +			* writethrough now.
> +			*/
> 			if (iocb->ki_flags & IOCB_SYNC) {
> 				dio->flags &= ~(IOMAP_DIO_WRITE_THROUGH |
> 						IOMAP_DIO_INLINE_COMP);
> 			}
>                  }
> 

[...]
> 
> However, this does mean that any spinlock taken in the ->end_io()
> callbacks now needs to be irq safe. e.g. in xfs_dio_write_end_io()
> the spinlock protection around inode size updates will need to use
> an irq safe locking, as will the locking in the DIO submission path
> that it serialises against in xfs_file_write_checks(). That probably
> is best implemented as a separate spinlock.
> 
> There will also be other filesystems that need to set IOMAP_F_DIRTY
> unconditionally (e.g. zonefs) because they always take blocking
> locks in their ->end_io callbacks and so must always run in task
> context...
Should we add a new flag(eg. IOMAP_F_ENDIO_IRQ ?) to indicate that the 
endio cannot be done under irq? Because I think IOMAP_F_DIRTY means that 
the metadata needs to be written, if we add a new semantics(endio must 
be done in defered work) for this flag, the code will looks a little 
complicated.