linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Selva Jove <selvajove@gmail.com>
Cc: SelvaKumar S <selvakuma.s1@samsung.com>,
	"linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
	"kbusch@kernel.org" <kbusch@kernel.org>,
	 "axboe@kernel.dk" <axboe@kernel.dk>, "hch@lst.de" <hch@lst.de>,
	"sagi@grimberg.me" <sagi@grimberg.me>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"dm-devel@redhat.com" <dm-devel@redhat.com>,
	"snitzer@redhat.com" <snitzer@redhat.com>,
	"joshiiitr@gmail.com" <joshiiitr@gmail.com>,
	"nj.shetty@samsung.com" <nj.shetty@samsung.com>,
	"joshi.k@samsung.com" <joshi.k@samsung.com>,
	"javier.gonz@samsung.com" <javier.gonz@samsung.com>,
	"kch@kernel.org" <kch@kernel.org>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC PATCH v5 2/4] block: add simple copy support
Date: Tue, 13 Apr 2021 00:32:14 +0000	[thread overview]
Message-ID: <BL0PR04MB65146169A9C7527280C15D4AE74F9@BL0PR04MB6514.namprd04.prod.outlook.com> (raw)
In-Reply-To: <CAHqX9vYvtOaVL4LG0gAGCMz+a8uha8czH==Dgg3eG+TWA+xeVQ@mail.gmail.com>

On 2021/04/12 23:35, Selva Jove wrote:
> On Mon, Apr 12, 2021 at 5:55 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>>
>> On 2021/04/07 20:33, Selva Jove wrote:
>>> Initially I started moving the dm-kcopyd interface to the block layer
>>> as a generic interface.
>>> Once I dig deeper in dm-kcopyd code, I figured that dm-kcopyd is
>>> tightly coupled with dm_io()
>>>
>>> To move dm-kcopyd to block layer, it would also require dm_io code to
>>> be moved to block layer.
>>> It would cause havoc in dm layer, as it is the backbone of the
>>> dm-layer and needs complete
>>> rewriting of dm-layer. Do you see any other way of doing this without
>>> having to move dm_io code
>>> or to have redundant code ?
>>
>> Right. Missed that. So reusing dm-kcopyd and making it a common interface will
>> take some more efforts. OK, then. For the first round of commits, let's forget
>> about this. But I still think that your emulation could be a lot better than a
>> loop doing blocking writes after blocking reads.
>>
> 
> Current implementation issues read asynchronously and once all the reads are
> completed, then the write is issued as whole to reduce the IO traffic
> in the queue.
> I agree that things can be better. Will explore another approach of
> sending writes
> immediately once reads are completed and with  plugging to increase the chances
> of merging.
> 
>> [...]
>>>>> +int blkdev_issue_copy(struct block_device *src_bdev, int nr_srcs,
>>>>> +             struct range_entry *src_rlist, struct block_device *dest_bdev,
>>>>> +             sector_t dest, gfp_t gfp_mask, int flags)
>>>>> +{
>>>>> +     struct request_queue *q = bdev_get_queue(src_bdev);
>>>>> +     struct request_queue *dest_q = bdev_get_queue(dest_bdev);
>>>>> +     struct blk_copy_payload *payload;
>>>>> +     sector_t bs_mask, copy_size;
>>>>> +     int ret;
>>>>> +
>>>>> +     ret = blk_prepare_payload(src_bdev, nr_srcs, src_rlist, gfp_mask,
>>>>> +                     &payload, &copy_size);
>>>>> +     if (ret)
>>>>> +             return ret;
>>>>> +
>>>>> +     bs_mask = (bdev_logical_block_size(dest_bdev) >> 9) - 1;
>>>>> +     if (dest & bs_mask) {
>>>>> +             return -EINVAL;
>>>>> +             goto out;
>>>>> +     }
>>>>> +
>>>>> +     if (q == dest_q && q->limits.copy_offload) {
>>>>> +             ret = blk_copy_offload(src_bdev, payload, dest, gfp_mask);
>>>>> +             if (ret)
>>>>> +                     goto out;
>>>>> +     } else if (flags & BLKDEV_COPY_NOEMULATION) {
>>>>
>>>> Why ? whoever calls blkdev_issue_copy() wants a copy to be done. Why would that
>>>> user say "Fail on me if the device does not support copy" ??? This is a weird
>>>> interface in my opinion.
>>>>
>>>
>>> BLKDEV_COPY_NOEMULATION flag was introduced to allow blkdev_issue_copy() callers
>>> to use their native copying method instead of the emulated copy that I
>>> added. This way we
>>> ensure that dm uses the hw-assisted copy and if that is not present,
>>> it falls back to existing
>>> copy method.
>>>
>>> The other users who don't have their native emulation can use this
>>> emulated-copy implementation.
>>
>> I do not understand. Emulation or not should be entirely driven by the device
>> reporting support for simple copy (or not). It does not matter which component
>> is issuing the simple copy call: an FS to a real device, and FS to a DM device
>> or a DM target driver. If the underlying device reported support for simple
>> copy, use that. Otherwise, emulate with read/write. What am I missing here ?
>>
> 
> blkdev_issue_copy() api will generally complete the copy-operation,
> either by using
> offloaded-copy or by using emulated-copy. The caller of the api is not
> required to
> figure the type of support. However, it can opt out of emulated-copy
> by specifying
> the flag BLKDEV_NOEMULATION. This is helpful for the case when the
> caller already
> has got a sophisticated emulation (e.g. dm-kcopyd users).

This does not make any sense to me. If the user has already another mean of
doing copies, then that user will not call blkdev_issue_copy(). So I really do
not understand what the "opting out of emulated copy" would be useful for. That
user can check the simple copy support glag in the device request queue and act
accordingly: use its own block copy code when simple copy is not supported or
use blkdev_issue_copy() when the device has simple copy. Adding that
BLKDEV_COPY_NOEMULATION does not serve any purpose at all.



-- 
Damien Le Moal
Western Digital Research

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

  reply	other threads:[~2021-04-13  0:34 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20210219124555epcas5p1334e7c4d64ada5dc4a2ca0feb48c1d44@epcas5p1.samsung.com>
2021-02-19 12:45 ` [RFC PATCH v5 0/4] " SelvaKumar S
     [not found]   ` <CGME20210219124559epcas5p41da46f1c248e334953d407a154697903@epcas5p4.samsung.com>
2021-02-19 12:45     ` [RFC PATCH v5 1/4] block: make bio_map_kern() non static SelvaKumar S
     [not found]   ` <CGME20210219124603epcas5p33add0f2c1781b2a4d71bf30c9e1ac647@epcas5p3.samsung.com>
2021-02-19 12:45     ` [RFC PATCH v5 2/4] block: add simple copy support SelvaKumar S
2021-02-20  4:59       ` Damien Le Moal
2021-04-07 11:32         ` Selva Jove
2021-04-12  0:24           ` Damien Le Moal
2021-04-12 14:34             ` Selva Jove
2021-04-13  0:32               ` Damien Le Moal [this message]
2021-04-14  6:58                 ` Selva Jove
     [not found]   ` <CGME20210219124608epcas5p2a673f9e00c3e7b5352f115497b0e2d98@epcas5p2.samsung.com>
2021-02-19 12:45     ` [RFC PATCH v5 3/4] nvme: " SelvaKumar S
2021-02-20  3:36       ` Matthew Wilcox
2021-02-22 15:57         ` Selva Jove
     [not found]   ` <CGME20210219124611epcas5p1c775b63b537e75da161556e375fcf05e@epcas5p1.samsung.com>
2021-02-19 12:45     ` [RFC PATCH v5 4/4] dm kcopyd: add simple copy offload support SelvaKumar S
2021-02-20 18:01   ` [RFC PATCH v5 0/4] add simple copy support David Laight
2021-02-20 19:08     ` Matthew Wilcox
2021-02-20 19:19     ` Keith Busch
2021-02-21 23:52   ` Dave Chinner
2021-02-23  9:14     ` Selva Jove
2021-02-22  1:31   ` Ming Lei
2021-02-22  6:52   ` Su Yue
2021-02-23  9:00     ` Selva Jove
2021-04-10  0:21   ` Max Gurtovoy
2021-04-10  0:29     ` Chaitanya Kulkarni
2021-04-10  6:32       ` Javier González
2021-04-11  9:10         ` Max Gurtovoy
2021-04-11 19:26           ` Javier González
2021-04-13 15:38             ` Max Gurtovoy
2021-04-13 18:25               ` Javier González
2021-04-13 18:36                 ` Chaitanya Kulkarni

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=BL0PR04MB65146169A9C7527280C15D4AE74F9@BL0PR04MB6514.namprd04.prod.outlook.com \
    --to=damien.lemoal@wdc.com \
    --cc=axboe@kernel.dk \
    --cc=dm-devel@redhat.com \
    --cc=hch@lst.de \
    --cc=javier.gonz@samsung.com \
    --cc=joshi.k@samsung.com \
    --cc=joshiiitr@gmail.com \
    --cc=kbusch@kernel.org \
    --cc=kch@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nvme@lists.infradead.org \
    --cc=nj.shetty@samsung.com \
    --cc=sagi@grimberg.me \
    --cc=selvajove@gmail.com \
    --cc=selvakuma.s1@samsung.com \
    --cc=snitzer@redhat.com \
    --subject='Re: [RFC PATCH v5 2/4] block: add simple copy support' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).