Linux Device Mapper development
 help / color / mirror / Atom feed
From: Mike Snitzer <snitzer@redhat.com>
To: Jeffle Xu <jefflexu@linux.alibaba.com>
Cc: joseph.qi@linux.alibaba.com, dm-devel@redhat.com, koct9i@gmail.com
Subject: Re: [dm-devel] dm: add support for DM_TARGET_NOWAIT for various targets
Date: Wed, 11 Nov 2020 10:38:24 -0500	[thread overview]
Message-ID: <20201111153824.GB22834@redhat.com> (raw)
In-Reply-To: <20201110065558.22694-1-jefflexu@linux.alibaba.com>

On Tue, Nov 10 2020 at  1:55am -0500,
Jeffle Xu <jefflexu@linux.alibaba.com> wrote:

> This is one prep patch for supporting iopoll for dm device.
> 
> The direct IO routine will set REQ_NOWAIT flag for REQ_HIPRI IO (that
> is, IO will do iopoll) in bio_set_polled(). Then in the IO submission
> routine, the ability of handling REQ_NOWAIT of the block device will
> be checked for REQ_HIPRI IO in submit_bio_checks(). -EOPNOTSUPP will
> be returned if the block device doesn't support REQ_NOWAIT.

submit_bio_checks() verifies the request_queue has QUEUE_FLAG_NOWAIT set
if the bio has REQ_NOWAIT.

> DM lacks support for REQ_NOWAIT until commit 6abc49468eea ("dm: add
> support for REQ_NOWAIT and enable it for linear target"). Since then,
> dm targets that support REQ_NOWAIT should advertise DM_TARGET_NOWAIT
> feature.

I'm not seeing why DM_TARGET_NOWAIT is needed (since you didn't add any
code that consumes the flag).

dm-table.c:dm_table_set_restrictions() has:

        if (dm_table_supports_nowait(t))
                blk_queue_flag_set(QUEUE_FLAG_NOWAIT, q);
        else
                blk_queue_flag_clear(QUEUE_FLAG_NOWAIT, q);

> This patch adds support for DM_TARGET_NOWAIT for those dm targets, the
> .map() algorithm of which just involves sector recalculation.

So you're looking to constrain which targets will properly support
REQ_NOWAIT, based on whether they do a simple remapping?


> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
> Hi Mike,
> 
> I could split these boilerplate code that each dm target have one
> seperate patch if you think that would be better.

One patch for all these is fine.  But it should include the code that I
assume you'll be adding to dm_table_supports_nowait() to further verify
that the targets in the table are all DM_TARGET_NOWAIT.

And why isn't dm-linear setting DM_TARGET_NOWAIT?

Also, other targets _could_ be made to support REQ_NOWAIT by
conditionally returning bio_wouldblock_error() if appropriate
(e.g. bio-based dm-multipath's case of queue_if_no_path).

Mike

> ---
>  drivers/md/dm-stripe.c   | 2 +-
>  drivers/md/dm-switch.c   | 1 +
>  drivers/md/dm-unstripe.c | 1 +
>  drivers/md/dm-zero.c     | 1 +
>  4 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/dm-stripe.c b/drivers/md/dm-stripe.c
> index 151d022b032d..df359d33cda8 100644
> --- a/drivers/md/dm-stripe.c
> +++ b/drivers/md/dm-stripe.c
> @@ -496,7 +496,7 @@ static void stripe_io_hints(struct dm_target *ti,
>  static struct target_type stripe_target = {
>  	.name   = "striped",
>  	.version = {1, 6, 0},
> -	.features = DM_TARGET_PASSES_INTEGRITY,
> +	.features = DM_TARGET_PASSES_INTEGRITY | DM_TARGET_NOWAIT,
>  	.module = THIS_MODULE,
>  	.ctr    = stripe_ctr,
>  	.dtr    = stripe_dtr,
> diff --git a/drivers/md/dm-switch.c b/drivers/md/dm-switch.c
> index bff4c7fa1cd2..262e2b0fd975 100644
> --- a/drivers/md/dm-switch.c
> +++ b/drivers/md/dm-switch.c
> @@ -550,6 +550,7 @@ static int switch_iterate_devices(struct dm_target *ti,
>  static struct target_type switch_target = {
>  	.name = "switch",
>  	.version = {1, 1, 0},
> +	.features = DM_TARGET_NOWAIT,
>  	.module = THIS_MODULE,
>  	.ctr = switch_ctr,
>  	.dtr = switch_dtr,
> diff --git a/drivers/md/dm-unstripe.c b/drivers/md/dm-unstripe.c
> index e673dacf6418..7357c1bd5863 100644
> --- a/drivers/md/dm-unstripe.c
> +++ b/drivers/md/dm-unstripe.c
> @@ -178,6 +178,7 @@ static void unstripe_io_hints(struct dm_target *ti,
>  static struct target_type unstripe_target = {
>  	.name = "unstriped",
>  	.version = {1, 1, 0},
> +	.features = DM_TARGET_NOWAIT,
>  	.module = THIS_MODULE,
>  	.ctr = unstripe_ctr,
>  	.dtr = unstripe_dtr,
> diff --git a/drivers/md/dm-zero.c b/drivers/md/dm-zero.c
> index b65ca8dcfbdc..faa1dbffc8b4 100644
> --- a/drivers/md/dm-zero.c
> +++ b/drivers/md/dm-zero.c
> @@ -59,6 +59,7 @@ static int zero_map(struct dm_target *ti, struct bio *bio)
>  static struct target_type zero_target = {
>  	.name   = "zero",
>  	.version = {1, 1, 0},
> +	.features = DM_TARGET_NOWAIT,
>  	.module = THIS_MODULE,
>  	.ctr    = zero_ctr,
>  	.map    = zero_map,
> -- 
> 2.27.0
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


  reply	other threads:[~2020-11-11 15:39 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-10  6:55 [dm-devel] [PATCH] dm: add support for DM_TARGET_NOWAIT for various targets Jeffle Xu
2020-11-11 15:38 ` Mike Snitzer [this message]
2020-11-12  6:05   ` [dm-devel] " JeffleXu
2020-11-12  7:58     ` JeffleXu
2020-11-12 16:11     ` Mike Snitzer
2020-11-13  2:05 ` [dm-devel] [PATCH v2] " Jeffle Xu
2020-11-13  2:12   ` JeffleXu
2020-11-18  2:01   ` JeffleXu
2020-11-18 15:38     ` Mike Snitzer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20201111153824.GB22834@redhat.com \
    --to=snitzer@redhat.com \
    --cc=dm-devel@redhat.com \
    --cc=jefflexu@linux.alibaba.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=koct9i@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox