Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
Date:   Fri, 8 Dec 2023 12:34:19 -0800 (PST)
From:   Eric Wheeler <dm-devel@lists.ewheeler.net>
To:     Hongyu Jin <hongyu.jin.cn@gmail.com>
cc:     Eric Biggers <ebiggers@kernel.org>, agk@redhat.com,
        snitzer@kernel.org, mpatocka@redhat.com, zhiguo.niu@unisoc.com,
        ke.wang@unisoc.com, yibin.ding@unisoc.com, hongyu.jin@unisoc.com,
        linux-kernel@vger.kernel.org, dm-devel@lists.linux.dev
Subject: Re: [PATCH v2] dm verity: Inherit I/O priority from data I/O when
 read FEC and hash from disk
In-Reply-To: <20231208015403.GB1160@sol.localdomain>
Message-ID: <df68c38e-3e38-eaf1-5c32-66e43d68cae3@ewheeler.net>
References: <83e8ea6c-1d9-36d5-1c23-da686dbfaf80@redhat.com> <20231206113935.9705-1-hongyu.jin.cn@gmail.com> <20231208015403.GB1160@sol.localdomain>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@unisoc.com>
> > 
> > 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@redhat.com/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@unisoc.com>
> > Signed-off-by: Yibin Ding <yibin.ding@unisoc.com>
> > Signed-off-by: Hongyu Jin <hongyu.jin@unisoc.com>
> > 
> > ---
> > 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
> 
>