On Thu, 7 Dec 2023, Eric Biggers wrote: > On Wed, Dec 06, 2023 at 07:39:35PM +0800, Hongyu Jin wrote: > > From: Hongyu Jin <hongyu.jin@xxxxxxxxxx> > > > > when read FEC and hash from disk, I/O priority are inconsistent > > with data block and blocked by other I/O with low I/O priority. > > > > Add dm_bufio_prefetch_by_ioprio() and dm_bufio_read_by_ioprio(), > > can pecific I/O priority for some I/O. > > Make I/O for FEC and hash has same I/O priority with data I/O. Hi Hongyu, +1 for the feature, thank you for cleaning up ioprio in device mapper! A few years ago we proposed a similar prior patch in dm-crypt; however, it was never committed, and I did not have the time to shepherd it through. Maybe this has since been addressed in some other way, or perhaps your work solves what we were doing with dm-crypt; either way, here is the link to that thread incase it is relevant to your work: https://www.mail-archive.com/dm-devel@xxxxxxxxxx/msg03828.html I look forward to seeing all (or at least the most common) device mapper targets cleanly support ioprio. Cheers, -- Eric Wheeler > > Co-developed-by: Yibin Ding <yibin.ding@xxxxxxxxxx> > > Signed-off-by: Yibin Ding <yibin.ding@xxxxxxxxxx> > > Signed-off-by: Hongyu Jin <hongyu.jin@xxxxxxxxxx> > > > > --- > > Changes in v2: > > - Add ioprio field in struct dm_io_region > > - Initial struct dm_io_region::ioprio to IOPRIO_DEFAULT > > - Add two interface > > --- > > drivers/md/dm-bufio.c | 50 ++++++++++++++++++++++----------- > > drivers/md/dm-integrity.c | 5 ++++ > > drivers/md/dm-io.c | 1 + > > drivers/md/dm-log.c | 1 + > > drivers/md/dm-raid1.c | 2 ++ > > drivers/md/dm-snap-persistent.c | 2 ++ > > drivers/md/dm-verity-fec.c | 3 +- > > drivers/md/dm-verity-target.c | 10 +++++-- > > drivers/md/dm-writecache.c | 4 +++ > > include/linux/dm-bufio.h | 6 ++++ > > include/linux/dm-io.h | 2 ++ > > 11 files changed, 66 insertions(+), 20 deletions(-) > > Changing so many things in one patch should be avoided if possible. Is there a > way to split this patch up? Maybe first add ioprio support to dm-io, then add > ioprio support to dm-bufio, then make dm-verity set the correct ioprio? > > > void *dm_bufio_read(struct dm_bufio_client *c, sector_t block, > > struct dm_buffer **bp) > > +{ > > + return dm_bufio_read_by_ioprio(c, block, bp, IOPRIO_DEFAULT); > > +} > > +EXPORT_SYMBOL_GPL(dm_bufio_read); > > + > > +void *dm_bufio_read_by_ioprio(struct dm_bufio_client *c, sector_t block, > > + struct dm_buffer **bp, unsigned short ioprio) > > { > > if (WARN_ON_ONCE(dm_bufio_in_request())) > > return ERR_PTR(-EINVAL); > > > > - return new_read(c, block, NF_READ, bp); > > + return new_read(c, block, NF_READ, bp, ioprio); > > } > > -EXPORT_SYMBOL_GPL(dm_bufio_read); > > +EXPORT_SYMBOL_GPL(dm_bufio_read_by_ioprio); > > > > void *dm_bufio_new(struct dm_bufio_client *c, sector_t block, > > struct dm_buffer **bp) > > @@ -1909,12 +1918,19 @@ void *dm_bufio_new(struct dm_bufio_client *c, sector_t block, > > if (WARN_ON_ONCE(dm_bufio_in_request())) > > return ERR_PTR(-EINVAL); > > > > - return new_read(c, block, NF_FRESH, bp); > > + return new_read(c, block, NF_FRESH, bp, IOPRIO_DEFAULT); > > } > > EXPORT_SYMBOL_GPL(dm_bufio_new); > > > > void dm_bufio_prefetch(struct dm_bufio_client *c, > > sector_t block, unsigned int n_blocks) > > +{ > > + return dm_bufio_prefetch_by_ioprio(c, block, n_blocks, IOPRIO_DEFAULT); > > +} > > +EXPORT_SYMBOL_GPL(dm_bufio_prefetch); > > + > > +void dm_bufio_prefetch_by_ioprio(struct dm_bufio_client *c, > > + sector_t block, unsigned int n_blocks, unsigned short ioprio) > > I think it would be cleaner to just add the ioprio parameter to dm_bufio_read() > and dm_bufio_prefetch(), instead of adding new functions. > > > diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c > > index 26adcfea0302..5945ac1dfdff 100644 > > --- a/drivers/md/dm-verity-target.c > > +++ b/drivers/md/dm-verity-target.c > > @@ -51,6 +51,7 @@ static DEFINE_STATIC_KEY_FALSE(use_tasklet_enabled); > > struct dm_verity_prefetch_work { > > struct work_struct work; > > struct dm_verity *v; > > + struct dm_verity_io *io; > > sector_t block; > > unsigned int n_blocks; > > }; > > Isn't it possible for 'io' to complete and be freed while the prefetch work is > still running? > > - Eric > >
