Messages in this thread | ![/](/images/icornerl.gif) | | Subject | Re: [PATCH RFC 1/2] iomap: Add a IOMAP_DIO_MAY_INLINE_COMP flag | From | Zhihao Cheng <> | Date | Mon, 11 Mar 2024 15:55:30 +0800 |
| |
在 2024/3/1 18:02, Zhihao Cheng 写道: > 在 2024/3/1 8:40, Dave Chinner 写道: > Hi Dave. Friendly ping. > 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. > > > .
| ![\](/images/icornerr.gif) |