linux-kernel - Re: [PATCH v2] dm verity: Inherit I/O priority from data I/O when read FEC and hash from disk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231208015403.GB1160@sol.localdomain>
Date:   Thu, 7 Dec 2023 17:54:03 -0800
From:   Eric Biggers <ebiggers@...nel.org>
To:     Hongyu Jin <hongyu.jin.cn@...il.com>
Cc:     agk@...hat.com, snitzer@...nel.org, mpatocka@...hat.com,
        zhiguo.niu@...soc.com, ke.wang@...soc.com, yibin.ding@...soc.com,
        hongyu.jin@...soc.com, linux-kernel@...r.kernel.org,
        dm-devel@...ts.linux.dev
Subject: Re: [PATCH v2] dm verity: Inherit I/O priority from data I/O when
 read FEC and hash from disk

On Wed, Dec 06, 2023 at 07:39:35PM +0800, Hongyu Jin wrote:
> From: Hongyu Jin <hongyu.jin@...soc.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.
> 
> Co-developed-by: Yibin Ding <yibin.ding@...soc.com>
> Signed-off-by: Yibin Ding <yibin.ding@...soc.com>
> Signed-off-by: Hongyu Jin <hongyu.jin@...soc.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