* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Pavel Begunkov @ 2026-05-18 9:11 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513083937.GD6461@lst.de>
On 5/13/26 09:39, Christoph Hellwig wrote:
>> + union {
>> + struct bio_vec *bi_io_vec;
>> + /* Driver specific dma map, present only with BIO_DMABUF_MAP */
>> + struct io_dmabuf_map *dmabuf_map;
>> + };
>
> ... and please add the bi_ prefix we're using for all (well except for
> one oddity) fields in struct bio.
Ok, going to add
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 06/10] block: forward create_dmabuf_token to drivers
From: Pavel Begunkov @ 2026-05-18 9:13 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513082553.GB6461@lst.de>
On 5/13/26 09:25, Christoph Hellwig wrote:
> On Wed, Apr 29, 2026 at 04:25:52PM +0100, Pavel Begunkov wrote:
>> Add a trivial implementation of the create_dmabuf_token call for
>> block devices that forwards the call to a new blk-mq callback if it's
>> available.
>
> This should go into block_device_operations as there is nothing blk-mq
> specific about this. I.e. even if this patchset doesn't handle stacking
> drivers yet, it should be easy enough to add them in the future.
Ok
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 02/10] iov_iter: add iterator type for dmabuf maps
From: Pavel Begunkov @ 2026-05-18 9:24 UTC (permalink / raw)
To: David Laight
Cc: Jens Axboe, Keith Busch, Christoph Hellwig, Sagi Grimberg,
Alexander Viro, Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513142909.03ae6c2b@pumpkin>
On 5/13/26 14:29, David Laight wrote:
> On Wed, 13 May 2026 11:05:57 +0100
> David Laight <david.laight.linux@gmail.com> wrote:
>
> ...
>>> @@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size)
>>> {
>>> if (unlikely(i->count < size))
>>> size = i->count;
>>> - if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {
>>> + if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i)) ||
>>> + unlikely(iov_iter_is_dmabuf_map(i))) {
>>
>>
>> Doesn't the extra check add more code to all the non-ubuf cases?
>> This could be fixed by either making iter_type a bitmask (with one bit set)
Not going to do that. It was specifically converted from bitmask
before, and the check optimisations like this were voiced back than.
>> or writing an iter_is_one_of(i, ITER_xxx, ITER_yyy) define that uses
>> '(1 << i->iter_type) & ((1 << ITER_xxx) | ...)'
>
> This seems to DTRT:
>
> #define _ITER_IS_ONE_OF(iter, t1, t2, t3, t4, t5, t6, t7, t8, ...) \
> ((1u << (iter)->iter_type) & ((1u << ITER_##t1) | (1u << ITER_##t2) | \
> (1u << ITER_##t3) | (1u << ITER_##t4) | (1u << ITER_##t5) | \
> (1u << ITER_##t6) | (1u << ITER_##t7) | (1u << ITER_##t8)))
> #define ITER_IS_ONE_OF(iter, t, ...) \
> _ITER_IS_ONE_OF(iter, t, ## __VA_ARGS__, t, t, t, t, t, t, t)
We definitely don't want that, using them directly would've been
much cleaner.
if (get_type_mask(i) & (TYPE1 | TYPE2)) ...
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 07/10] nvme-pci: implement dma_token backed requests
From: Pavel Begunkov @ 2026-05-18 9:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513083817.GC6461@lst.de>
On 5/13/26 09:38, Christoph Hellwig wrote:
> FYI, I really want SGL support before this get merged, but ignoring that
> for now:
I was hoping to let Samsung guys to send a follow up they already have,
but I'll ask them to have about taking it into this patch set.
>> +struct nvme_dmabuf_map {
>> + struct io_dmabuf_map base;
>> + dma_addr_t *dma_list;
>> + struct sg_table *sgt;
>> + unsigned nr_entries;
>
> I'd make dma_list a variable-sized array at the end of the struture to avoid
> an extra allocation and pointer derefernece.
Ok
>> +static void nvme_dmabuf_map_sync(struct nvme_dev *nvme_dev, struct request *req,
>> + bool for_cpu)
>> +{
>> + int length = blk_rq_payload_bytes(req);
>> + struct device *dev = nvme_dev->dev;
>> + enum dma_data_direction dma_dir;
>> + struct bio *bio = req->bio;
>> + struct nvme_dmabuf_map *map;
>> + dma_addr_t *dma_list;
>> + int offset, map_idx;
>> +
>> + dma_dir = rq_data_dir(req) == READ ? DMA_FROM_DEVICE : DMA_TO_DEVICE;
>> + map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
>> + dma_list = map->dma_list;
>> +
>> + offset = bio->bi_iter.bi_bvec_done;
>> + map_idx = offset / NVME_CTRL_PAGE_SIZE;
>> + length += offset & (NVME_CTRL_PAGE_SIZE - 1);
>
> Please initialize the variable at declaration time and use or add proper
> helpers to simplify this:
> static inline struct nvme_dmabuf_map *
> to_nvme_dmabuf_map(struct io_dmabuf_map *map)
> {
> return container_of(map, struct nvme_dmabuf_map, base);
> }
>
> ....
>
> enum dma_data_direction dma_dir = rq_dma_dir(req);
> struct device *dev = nvme_dev->dev;
> struct bio *bio = req->bio;
> struct nvme_dmabuf_map *map = to_nvme_dmabuf_map(bio->bi_dmabuf_map);
> dma_addr_t *dma_list = map->dma_list;
> int offset = bio->bi_iter.bi_bvec_done;
> int mmap_idx = offset / NVME_CTRL_PAGE_SIZE;
> int length = blk_rq_payload_bytes(req) +
> offset & (NVME_CTRL_PAGE_SIZE - 1);
>
> Also a lot of these ints sound like they should be unsigned.
Ok
>> +
>> + while (length > 0) {
>> + u64 dma_addr = dma_list[map_idx++];
>> +
>> + if (for_cpu)
>> + __dma_sync_single_for_cpu(dev, dma_addr,
>> + NVME_CTRL_PAGE_SIZE, dma_dir);
>> + else
>> + __dma_sync_single_for_device(dev, dma_addr,
>> + NVME_CTRL_PAGE_SIZE,
>> + dma_dir);
>> + length -= NVME_CTRL_PAGE_SIZE;
>> + }
>> +}
>
> Nothing should be using these __dma_sync helpers that are internal
> details. Using them means you call into sync code that should be skipped
> on most common server class systems.
Yeah, the kernel test robot already flagged it as well
> Also the for_cpu argument is a bit ugly. I'd rather have separate
> routines as in the core dma-mapping code, even if that means a little bit
> of code duplication.
>
>> +static blk_status_t nvme_rq_setup_dmabuf_map(struct request *req,
>> + struct nvme_queue *nvmeq)
>> +{
>> + struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
>> + int length = blk_rq_payload_bytes(req);
>> + u64 dma_addr, prp1_dma, prp2_dma;
>> + struct bio *bio = req->bio;
>> + struct nvme_dmabuf_map *map;
>> + dma_addr_t *dma_list;
>> + dma_addr_t prp_dma;
>> + __le64 *prp_list;
>> + int i, map_idx;
>> + int offset;
>> +
>> + nvme_dmabuf_map_sync(nvmeq->dev, req, false);
>> +
>> + map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
>> + dma_list = map->dma_list;
>> +
>> + offset = bio->bi_iter.bi_bvec_done;
>> + map_idx = offset / NVME_CTRL_PAGE_SIZE;
>> + offset &= (NVME_CTRL_PAGE_SIZE - 1);
>> + prp1_dma = dma_list[map_idx++] + offset;
>
> Same comments as for the sync helper above.
>
>> + length -= (NVME_CTRL_PAGE_SIZE - offset);
>> + if (length <= 0) {
>> + prp2_dma = 0;
>> + goto done;
>> + }
>> +
>> + if (length <= NVME_CTRL_PAGE_SIZE) {
>> + prp2_dma = dma_list[map_idx];
>> + goto done;
>> + }
>> +
>> + if (DIV_ROUND_UP(length, NVME_CTRL_PAGE_SIZE) <=
>> + NVME_SMALL_POOL_SIZE / sizeof(__le64))
>> + iod->flags |= IOD_SMALL_DESCRIPTOR;
>> +
>> + prp_list = dma_pool_alloc(nvme_dma_pool(nvmeq, iod), GFP_ATOMIC,
>> + &prp_dma);
>> + if (!prp_list)
>> + return BLK_STS_RESOURCE;
>> +
>> + iod->descriptors[iod->nr_descriptors++] = prp_list;
>> + prp2_dma = prp_dma;
>
> And I really hate how this duplicates all the nasty PRP building logic,
> although right now I don't have a good answer to that.
>
>> +static inline bool nvme_rq_is_dmabuf_attached(struct request *req)
>> +{
>> + if (!IS_ENABLED(CONFIG_DMABUF_TOKEN))
>> + return false;
>> + return req->bio && bio_flagged(req->bio, BIO_DMABUF_MAP);
>> +}
>
> This is something that should go into the block layer.
I'll move it
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH 1/2] direct-io: remove IOCB_NOWAIT support
From: Damien Le Moal @ 2026-05-18 9:34 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Christian Brauner, Jan Kara, linux-fsdevel, linux-block
In-Reply-To: <20260518063336.507369-2-hch@lst.de>
On 2026/05/18 8:33, Christoph Hellwig wrote:
> None of the file systems using the legacy direct I/O code actually sets
> FMODE_NOWAIT, and if they did this would not work, as the write locking
> could not handle the retry. Remove this dead code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply
* Re: [PATCH 2/2] block: don't set BIO_QUIET for BLK_STS_AGAIN
From: Damien Le Moal @ 2026-05-18 9:34 UTC (permalink / raw)
To: Christoph Hellwig, Jens Axboe
Cc: Christian Brauner, Jan Kara, linux-fsdevel, linux-block
In-Reply-To: <20260518063336.507369-3-hch@lst.de>
On 2026/05/18 8:33, Christoph Hellwig wrote:
> Commit abb30460bda2 ("block: mark bio_wouldblock_error() bio with
> BIO_QUIET") added this to suppress buffer_head warnings, but neither
> when this commit was added nor now any buffer_head using code actually
> ever sets REQ_NOWAIT which can lead to BLK_STS_AGAIN.
>
> Remove the special handling for now. If we ever plan to use REQ_NOWAIT
> for buffer_head based I/O we're better off handling BLK_STS_AGAIN in
> the completion handler as it actually needs to retry the I/O as well.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply
* Re: [PATCH v3 05/10] lib: add dmabuf token infrastructure
From: Pavel Begunkov @ 2026-05-18 10:14 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513082431.GA6461@lst.de>
On 5/13/26 09:24, Christoph Hellwig wrote:
> Naming and placement:
>
> This is about dma-buf based I/O. So I'd expect it to be named dma-buf-io
> and no io-dmabuf, and live in drivers/dma-buf and not the unrelated lib/.
> But I'd like to hear from the dma-buf maintainers about that.
Looking at what Ming is saying, it'd make more sense to keep some of the
parts like iterator and the file op more flexible and not automatically
imply dma-buf even if it's the main and for now the only medium. I.e.
ublk/fuse can use a similar interface for mapping buffers to the server
even without dma mappings.
I don't know how the API should look like, maybe passing memfd, and dma-buf
supports mmap, but I think it's better to call the op something like
"register_buffer" instead and keep all it in lib/ for the same reasons.
> Config option: as this unconditionally when DMA_SHARED_BUFFER is enabled,
> why does it need a separate config option?
More clearly marking relevant code, easier to make optional if needed,
and gives some introspection via /proc/config.
> Interface: io_dmabuf_token_create / ->create_dmabuf_token filling
> in a structure allocated by the caller feels odd.
It's minimising pointer chasing. "token" is mainly used by io_uring in
the hot path, and io_uring just keeps it as a part of a larger struct.
For the same reasons "map" is allocated by the driver.
I can add an extra parameter to io_dmabuf_token_create() for how
many extra bytes to allocate for the caller's use, if that makes
things any better for you, but it was easier to just pass an
already allocated struct.
My gut feeling
> would be to move most of io_dmabuf_token_create into a helper called
> by ->create_dmabuf_token so that the token is allocated in the
> driver data structure and returned from create_dmabuf_token.
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 07/10] nvme-pci: implement dma_token backed requests
From: Anuj Gupta/Anuj Gupta @ 2026-05-18 10:18 UTC (permalink / raw)
To: Pavel Begunkov, Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Jason Gunthorpe
In-Reply-To: <50ed7240-d8d3-4816-bcc9-ce8adbbbf841@gmail.com>
On 5/18/2026 2:59 PM, Pavel Begunkov wrote:
>> FYI, I really want SGL support before this get merged, but ignoring that
>> for now:
>
> I was hoping to let Samsung guys to send a follow up they already have,
> but I'll ask them to have about taking it into this patch set.
I had done patches on top of v3 adding SGL support and PRP list reuse
optimization for the dmabuf path.
Branch: https://github.com/SamsungDS/linux/commits/rw-dmabuf-v3-nvme-opt/
Also pasting the SGL patch here for quick reference:
Subject: [PATCH 1/2] nvme-pci: add sgl support for dmabuf path
Handle dmabuf-backed requests through the SGL setup path too.
Use the cached dmabuf sg_table and keep PRP fallback where allowed.
Signed-off-by: Anuj Gupta <anuj20.g@samsung.com>
---
drivers/nvme/host/pci.c | 194 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 193 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0a49c94dd675..31e37ab8769b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1270,6 +1270,14 @@ static void nvme_pci_sgl_set_data(struct
nvme_sgl_desc *sge,
sge->type = NVME_SGL_FMT_DATA_DESC << 4;
}
+static void nvme_pci_sgl_set_data_addr(struct nvme_sgl_desc *sge,
+ dma_addr_t addr, u32 len)
+{
+ sge->addr = cpu_to_le64(addr);
+ sge->length = cpu_to_le32(len);
+ sge->type = NVME_SGL_FMT_DATA_DESC << 4;
+}
+
static void nvme_pci_sgl_set_seg(struct nvme_sgl_desc *sge,
dma_addr_t dma_addr, int entries)
{
@@ -1321,6 +1329,176 @@ static blk_status_t
nvme_pci_setup_data_sgl(struct request *req,
return iter->status;
}
+static unsigned int nvme_pci_dmabuf_sgl_nents(struct request *req,
+ dma_addr_t *first_dma, u32 *first_len)
+{
+ struct bio *bio = req->bio;
+ struct nvme_dmabuf_map *map;
+ struct scatterlist *sg;
+ unsigned long tmp;
+ size_t offset = bio->bi_iter.bi_bvec_done;
+ size_t remaining = blk_rq_payload_bytes(req);
+ dma_addr_t last_end = 0;
+ unsigned int nents = 0;
+ dma_addr_t dma = 0;
+ u32 len = 0;
+ bool have = false;
+
+ map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+
+ for_each_sgtable_dma_sg(map->sgt, sg, tmp) {
+ size_t sg_len = sg_dma_len(sg);
+ dma_addr_t addr = sg_dma_address(sg);
+
+ if (!remaining)
+ break;
+ if (offset >= sg_len) {
+ offset -= sg_len;
+ continue;
+ }
+
+ addr += offset;
+ sg_len -= offset;
+ offset = 0;
+
+ while (sg_len && remaining) {
+ u32 chunk = min_t(size_t, remaining, sg_len);
+
+ if (!have || last_end != addr) {
+ nents++;
+ if (nents == 1) {
+ dma = addr;
+ len = chunk;
+ }
+ } else if (nents == 1) {
+ len += chunk;
+ }
+
+ have = true;
+ last_end = addr + chunk;
+ addr += chunk;
+ sg_len -= chunk;
+ remaining -= chunk;
+ }
+ }
+
+ if (unlikely(remaining))
+ return 0;
+
+ *first_dma = dma;
+ *first_len = len;
+ return nents;
+}
+
+static unsigned int nvme_pci_dmabuf_avg_seg_size(struct request *req)
+{
+ dma_addr_t first_dma;
+ u32 first_len;
+ unsigned int nseg;
+
+ nseg = nvme_pci_dmabuf_sgl_nents(req, &first_dma, &first_len);
+ if (!nseg)
+ return 0;
+ return DIV_ROUND_UP(blk_rq_payload_bytes(req), nseg);
+}
+
+static blk_status_t nvme_rq_setup_dmabuf_sgl(struct request *req,
+ struct nvme_queue *nvmeq)
+{
+ struct nvme_iod *iod = blk_mq_rq_to_pdu(req);
+ struct bio *bio = req->bio;
+ struct nvme_dmabuf_map *map;
+ size_t length = blk_rq_payload_bytes(req);
+ struct nvme_sgl_desc *sg_list = NULL;
+ dma_addr_t sgl_dma = 0, first_dma, last_end = 0;
+ unsigned int entries, mapped = 0;
+ unsigned long tmp;
+ struct scatterlist *sg;
+ size_t offset, remaining;
+ u32 first_len;
+ bool have = false;
+
+ map = container_of(bio->dmabuf_map, struct nvme_dmabuf_map, base);
+
+ entries = nvme_pci_dmabuf_sgl_nents(req, &first_dma, &first_len);
+ if (!entries)
+ return BLK_STS_IOERR;
+ if (entries > NVME_MAX_SEGS)
+ return BLK_STS_AGAIN;
+
+ iod->cmd.common.flags = NVME_CMD_SGL_METABUF;
+ iod->total_len = length;
+
+ nvme_sync_dma(nvmeq->dev, req, false);
+
+ if (entries == 1) {
+ nvme_pci_sgl_set_data_addr(&iod->cmd.common.dptr.sgl,
first_dma,
+ first_len);
+ return BLK_STS_OK;
+ }
+
+ if (entries <= NVME_SMALL_POOL_SIZE / sizeof(*sg_list))
+ iod->flags |= IOD_SMALL_DESCRIPTOR;
+
+ sg_list = dma_pool_alloc(nvme_dma_pool(nvmeq, iod), GFP_ATOMIC,
+ &sgl_dma);
+ if (!sg_list)
+ return BLK_STS_RESOURCE;
+ iod->descriptors[iod->nr_descriptors++] = sg_list;
+
+ offset = bio->bi_iter.bi_bvec_done;
+ remaining = length;
+
+ for_each_sgtable_dma_sg(map->sgt, sg, tmp) {
+ size_t sg_len = sg_dma_len(sg);
+ dma_addr_t addr = sg_dma_address(sg);
+
+ if (!remaining)
+ break;
+ if (offset >= sg_len) {
+ offset -= sg_len;
+ continue;
+ }
+
+ addr += offset;
+ sg_len -= offset;
+ offset = 0;
+
+ while (sg_len && remaining) {
+ u32 chunk = min_t(size_t, remaining, sg_len);
+
+ if (have && last_end == addr) {
+ u32 old = le32_to_cpu(sg_list[mapped -
1].length);
+
+ sg_list[mapped - 1].length =
+ cpu_to_le32(old + chunk);
+ } else {
+ if (WARN_ON_ONCE(mapped == entries))
+ goto err_free;
+
nvme_pci_sgl_set_data_addr(&sg_list[mapped++],
+ addr, chunk);
+ }
+
+ have = true;
+ last_end = addr + chunk;
+ addr += chunk;
+ sg_len -= chunk;
+ remaining -= chunk;
+ }
+ }
+
+ if (unlikely(remaining))
+ goto err_free;
+
+ nvme_pci_sgl_set_seg(&iod->cmd.common.dptr.sgl, sgl_dma, mapped);
+ return BLK_STS_OK;
+
+err_free:
+ iod->nr_descriptors--;
+ dma_pool_free(nvme_dma_pool(nvmeq, iod), sg_list, sgl_dma);
+ return BLK_STS_IOERR;
+}
+
static blk_status_t nvme_pci_setup_data_simple(struct request *req,
enum nvme_use_sgl use_sgl)
{
@@ -1369,8 +1547,22 @@ static blk_status_t nvme_map_data(struct request
*req)
struct blk_dma_iter iter;
blk_status_t ret;
- if (nvme_rq_is_dmabuf_attached(req))
+ if (nvme_rq_is_dmabuf_attached(req)) {
+ if (use_sgl == SGL_FORCED) {
+ ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq);
+ /* Regular path doesn't fall back if SGLs are
forced. */
+ return ret == BLK_STS_AGAIN ? BLK_STS_IOERR : ret;
+ }
+
+ if (use_sgl == SGL_SUPPORTED && sgl_threshold &&
+ nvme_pci_dmabuf_avg_seg_size(req) >= sgl_threshold) {
+ ret = nvme_rq_setup_dmabuf_sgl(req, nvmeq);
+ if (ret != BLK_STS_AGAIN)
+ return ret;
+ }
+
return nvme_rq_setup_dmabuf_map(req, nvmeq);
+ }
/*
* Try to skip the DMA iterator for single segment requests, as
that
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Pavel Begunkov @ 2026-05-18 10:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <20260513081929.GD5477@lst.de>
On 5/13/26 09:19, Christoph Hellwig wrote:
>> + if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
>> + bio->bi_io_vec = bio_src->bi_io_vec;
>> + } else {
>> + bio->dmabuf_map = bio_src->dmabuf_map;
>> + bio_set_flag(bio, BIO_DMABUF_MAP);
>> + }
>
> This is backwards, please avoid pointless negations:
I can flip it, but compilers tend to prefer the true branch. E.g. this
if (cond) A; else B;
C;
can get compiled into:
jmpcc cond B
A: ...
C:
return;
B: ...
jmp C;
>
> if (bio_flagged(bio_src, BIO_DMABUF_MAP)) {
> bio->dmabuf_map = bio_src->dmabuf_map;
> bio_set_flag(bio, BIO_DMABUF_MAP);
> } else {
> bio->bi_io_vec = bio_src->bi_io_vec;
> }
>
>> + if (bio_flagged(bio, BIO_DMABUF_MAP)) {
>> + nsegs = 1;
>> +
>> + if ((bio->bi_iter.bi_bvec_done & lim->dma_alignment) ||
>> + (bio->bi_iter.bi_size & len_align_mask))
>> + return -EINVAL;
>> + if (bio->bi_iter.bi_size > max_bytes) {
>> + bytes = max_bytes;
>> + goto split;
>> + }
>
> Please add a comment explaining why nsegs is always 1 here.
>
>> @@ -424,7 +424,8 @@ static inline struct bio *__bio_split_to_limits(struct bio *bio,
>> switch (bio_op(bio)) {
>> case REQ_OP_READ:
>> case REQ_OP_WRITE:
>> - if (bio_may_need_split(bio, lim))
>> + if (bio_may_need_split(bio, lim) ||
>> + bio_flagged(bio, BIO_DMABUF_MAP))
>> return bio_split_rw(bio, lim, nr_segs);
>
> The BIO_DMABUF_MAP check should go into bio_may_need_split.
Ok
>> +static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
>> + unsigned int bytes)
>> +{
>> + iter->bi_bvec_done += bytes;
>> + iter->bi_size -= bytes;
>> +}
>> +
>> static inline void bio_advance_iter(const struct bio *bio,
>> struct bvec_iter *iter, unsigned int bytes)
>> {
>> iter->bi_sector += bytes >> 9;
>>
>> - if (bio_no_advance_iter(bio))
>> + if (bio_no_advance_iter(bio)) {
>> iter->bi_size -= bytes;
>> - else
>> + } else if (bio_flagged(bio, BIO_DMABUF_MAP)) {
>> + bio_advance_iter_dmabuf_map(iter, bytes);
>
> This is a bit of a mess. You're using bi_bvec_done for something that
> is not bvec_done, which makes the naming very confusing. That is even
> more confusing than the existing usage, which isn't great. Also we
> add yet another conditional to heavily inlined code. I'd suggest
> the following:
>
> - add a prep patch to rename bi_bvec_done to bi_offset, as even for
> the existing usage it is the offset into the current bio_vec as
> much as it is the count of byes done, as those must be the same
> and it is used both ways
> - add a prep patch to also increase bi_offset for bio_no_advance_iter.
> It is not actually use there, but incrementing it is harmless and
> this will avoid a new special case
> - please also documet this new usage in the commet in struct bvec_iter.
> - then just add the dma buf mapping to the bio_no_advance_iter condition
I'll take a look
> - figure out what to do about dm_bio_rewind_iter, which pokes into these
> things that really should be block layer internal
Need to check what that is, but doesn't implement the interface and
is not supposed to ever see the dmabuf iterator.
>> }
>> @@ -391,7 +403,7 @@ static inline void bio_wouldblock_error(struct bio *bio)
>> */
>> static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
>> {
>> - if (iov_iter_is_bvec(iter))
>> + if (iov_iter_is_bvec(iter) || iov_iter_is_dmabuf_map(iter))
>> return 0;
>> return iov_iter_npages(iter, max_segs);
>> }
>
> Please update the comment for this helper.
>
>> @@ -322,6 +327,7 @@ enum {
>> BIO_REMAPPED,
>> BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
>> BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
>> + BIO_DMABUF_MAP, /* Using premmaped dma buffers */
>
> Shouldn't this be a REQ_ flag as we should never mix and match bios with
> and without this flag in a single request?
Do you mean adding both and propagating it from bio to req? submit_bio()
takes a bio, so we still need to set it there before it reaches blk-mq.
And there might be bio-based drivers using it in the future.
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 07/10] nvme-pci: implement dma_token backed requests
From: Pavel Begunkov @ 2026-05-18 10:30 UTC (permalink / raw)
To: Anuj Gupta/Anuj Gupta, Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Jason Gunthorpe
In-Reply-To: <f9e04625-50c6-4fa9-8b12-76496e29f10c@samsung.com>
On 5/18/26 11:18, Anuj Gupta/Anuj Gupta wrote:
> On 5/18/2026 2:59 PM, Pavel Begunkov wrote:
>>> FYI, I really want SGL support before this get merged, but ignoring that
>>> for now:
>>
>> I was hoping to let Samsung guys to send a follow up they already have,
>> but I'll ask them to have about taking it into this patch set.
>
> I had done patches on top of v3 adding SGL support and PRP list reuse
> optimization for the dmabuf path.
> Branch: https://github.com/SamsungDS/linux/commits/rw-dmabuf-v3-nvme-opt/
>
> Also pasting the SGL patch here for quick reference:
Thanks Anuj!
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH v3 02/10] iov_iter: add iterator type for dmabuf maps
From: David Laight @ 2026-05-18 10:40 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Jens Axboe, Keith Busch, Christoph Hellwig, Sagi Grimberg,
Alexander Viro, Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <4b2f74e9-3225-47f6-85fe-911720030e35@gmail.com>
On Mon, 18 May 2026 10:24:35 +0100
Pavel Begunkov <asml.silence@gmail.com> wrote:
> On 5/13/26 14:29, David Laight wrote:
> > On Wed, 13 May 2026 11:05:57 +0100
> > David Laight <david.laight.linux@gmail.com> wrote:
> >
> > ...
> >>> @@ -575,7 +575,8 @@ void iov_iter_advance(struct iov_iter *i, size_t size)
> >>> {
> >>> if (unlikely(i->count < size))
> >>> size = i->count;
> >>> - if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i))) {
> >>> + if (likely(iter_is_ubuf(i)) || unlikely(iov_iter_is_xarray(i)) ||
> >>> + unlikely(iov_iter_is_dmabuf_map(i))) {
> >>
> >>
> >> Doesn't the extra check add more code to all the non-ubuf cases?
...
> >> or writing an iter_is_one_of(i, ITER_xxx, ITER_yyy) define that uses
> >> '(1 << i->iter_type) & ((1 << ITER_xxx) | ...)'
> >
> > This seems to DTRT:
> >
> > #define _ITER_IS_ONE_OF(iter, t1, t2, t3, t4, t5, t6, t7, t8, ...) \
> > ((1u << (iter)->iter_type) & ((1u << ITER_##t1) | (1u << ITER_##t2) | \
> > (1u << ITER_##t3) | (1u << ITER_##t4) | (1u << ITER_##t5) | \
> > (1u << ITER_##t6) | (1u << ITER_##t7) | (1u << ITER_##t8)))
> > #define ITER_IS_ONE_OF(iter, t, ...) \
> > _ITER_IS_ONE_OF(iter, t, ## __VA_ARGS__, t, t, t, t, t, t, t)
>
> We definitely don't want that, using them directly would've been
> much cleaner.
>
> if (get_type_mask(i) & (TYPE1 | TYPE2)) ...
You need to shift all the TYPEn as well.
The above condition would become:
if (likely(ITER_IS_ONE_OF(i, UBUF, XARRAY, DMABUF_MAP))) {
which reads reasonable well.
Without the token pasting it becomes:
if (likely(ITER_IS_ONE_OF(i, ITER_UBUF, ITER_XARRAY, ITER_DMABUF_MAP))) {
and the line starts getting long.
Although that version probably needs a check that the mask is constant.
-- David
>
^ permalink raw reply
* Re: [PATCH v3 05/10] lib: add dmabuf token infrastructure
From: Markus Elfring @ 2026-05-18 11:24 UTC (permalink / raw)
To: Pavel Begunkov, linux-block, linux-fsdevel, linux-media, io-uring,
linux-nvme, linaro-mm-sig, dri-devel, Alexander Viro,
Andrew Morton, Christian Brauner, Christian König,
Christoph Hellwig, Jens Axboe, Keith Busch, Sagi Grimberg,
Sumit Semwal
Cc: LKML, Anuj Gupta, Jason Gunthorpe, Kanchan Joshi, Kees Cook,
Nitesh Shetty, Phil Cayton, Tushar Gohad, William Power
In-Reply-To: <c61e6d928f86f4cb253ae350272e6039faefd3a6.1777475843.git.asml.silence@gmail.com>
…
> +++ b/lib/io_dmabuf_token.c
> @@ -0,0 +1,272 @@
…
> +int io_dmabuf_init_map(struct io_dmabuf_token *token, struct io_dmabuf_map *map)
> +{
> + struct io_dmabuf_fence *fence = NULL;
> + int ret;
> +
> + fence = kzalloc(sizeof(*fence), GFP_KERNEL);
How do you think about to use kzalloc_obj() instead?
https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/slab.h#L1039-L1040
…
> + if (!fence)
> + return -ENOMEM;
> +
> + ret = percpu_ref_init(&map->refs, io_dmabuf_map_refs_release, 0, GFP_KERNEL);
> + if (ret) {
> + kfree(fence);
> + return ret;
> + }
…
Would you like to use the attribute “__free(kfree)”?
https://elixir.bootlin.com/linux/v7.1-rc3/source/include/linux/slab.h#L517
Regards,
Markus
^ permalink raw reply
* Re: [PATCH 1/2] direct-io: remove IOCB_NOWAIT support
From: Jan Kara @ 2026-05-18 12:09 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Christian Brauner, Jan Kara, linux-fsdevel,
linux-block
In-Reply-To: <20260518063336.507369-2-hch@lst.de>
On Mon 18-05-26 08:33:29, Christoph Hellwig wrote:
> None of the file systems using the legacy direct I/O code actually sets
> FMODE_NOWAIT, and if they did this would not work, as the write locking
> could not handle the retry. Remove this dead code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
I agree there's no point in bothering with this. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/direct-io.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 2267f5ae7f77..9e4976716985 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -502,12 +502,8 @@ static blk_status_t dio_bio_complete(struct dio *dio, struct bio *bio)
> const enum req_op dio_op = dio->opf & REQ_OP_MASK;
> bool should_dirty = dio_op == REQ_OP_READ && dio->should_dirty;
>
> - if (err) {
> - if (err == BLK_STS_AGAIN && (bio->bi_opf & REQ_NOWAIT))
> - dio->io_error = -EAGAIN;
> - else
> - dio->io_error = -EIO;
> - }
> + if (err)
> + dio->io_error = -EIO;
>
> if (dio->is_async && should_dirty) {
> bio_check_pages_dirty(bio); /* transfers ownership */
> @@ -1178,13 +1174,10 @@ ssize_t __blockdev_direct_IO(struct kiocb *iocb, struct inode *inode,
> dio->is_async = true;
>
> dio->inode = inode;
> - if (iov_iter_rw(iter) == WRITE) {
> + if (iov_iter_rw(iter) == WRITE)
> dio->opf = REQ_OP_WRITE | REQ_SYNC | REQ_IDLE;
> - if (iocb->ki_flags & IOCB_NOWAIT)
> - dio->opf |= REQ_NOWAIT;
> - } else {
> + else
> dio->opf = REQ_OP_READ;
> - }
>
> /*
> * For AIO O_(D)SYNC writes we need to defer completions to a workqueue
> --
> 2.53.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Christian König @ 2026-05-18 12:22 UTC (permalink / raw)
To: Pavel Begunkov, Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal, linux-block,
linux-kernel, linux-nvme, linux-fsdevel, io-uring, linux-media,
dri-devel, linaro-mm-sig, Nitesh Shetty, Kanchan Joshi,
Anuj Gupta, Tushar Gohad, William Power, Phil Cayton,
Jason Gunthorpe
In-Reply-To: <24833f76-2289-4859-86d1-9215b11a1258@gmail.com>
On 5/18/26 12:29, Pavel Begunkov wrote:
> On 5/13/26 09:19, Christoph Hellwig wrote:
>>> + if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
>>> + bio->bi_io_vec = bio_src->bi_io_vec;
>>> + } else {
>>> + bio->dmabuf_map = bio_src->dmabuf_map;
>>> + bio_set_flag(bio, BIO_DMABUF_MAP);
>>> + }
>>
>> This is backwards, please avoid pointless negations:
>
> I can flip it, but compilers tend to prefer the true branch. E.g. this
>
> if (cond) A; else B;
> C;
>
> can get compiled into:
>
> jmpcc cond B
> A: ...
> C:
> return;
> B: ...
> jmp C;
When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.
What could be useful is to have the true path for the more common and the false path for the less common case for documentation purposes, but in that case I would expected some code comments as well.
Regards,
Christian.
>
>
>>
>> if (bio_flagged(bio_src, BIO_DMABUF_MAP)) {
>> bio->dmabuf_map = bio_src->dmabuf_map;
>> bio_set_flag(bio, BIO_DMABUF_MAP);
>> } else {
>> bio->bi_io_vec = bio_src->bi_io_vec;
>> }
>>
>>> + if (bio_flagged(bio, BIO_DMABUF_MAP)) {
>>> + nsegs = 1;
>>> +
>>> + if ((bio->bi_iter.bi_bvec_done & lim->dma_alignment) ||
>>> + (bio->bi_iter.bi_size & len_align_mask))
>>> + return -EINVAL;
>>> + if (bio->bi_iter.bi_size > max_bytes) {
>>> + bytes = max_bytes;
>>> + goto split;
>>> + }
>>
>> Please add a comment explaining why nsegs is always 1 here.
>
>
>
>>
>>> @@ -424,7 +424,8 @@ static inline struct bio *__bio_split_to_limits(struct bio *bio,
>>> switch (bio_op(bio)) {
>>> case REQ_OP_READ:
>>> case REQ_OP_WRITE:
>>> - if (bio_may_need_split(bio, lim))
>>> + if (bio_may_need_split(bio, lim) ||
>>> + bio_flagged(bio, BIO_DMABUF_MAP))
>>> return bio_split_rw(bio, lim, nr_segs);
>>
>> The BIO_DMABUF_MAP check should go into bio_may_need_split.
>
> Ok
>>> +static inline void bio_advance_iter_dmabuf_map(struct bvec_iter *iter,
>>> + unsigned int bytes)
>>> +{
>>> + iter->bi_bvec_done += bytes;
>>> + iter->bi_size -= bytes;
>>> +}
>>> +
>>> static inline void bio_advance_iter(const struct bio *bio,
>>> struct bvec_iter *iter, unsigned int bytes)
>>> {
>>> iter->bi_sector += bytes >> 9;
>>> - if (bio_no_advance_iter(bio))
>>> + if (bio_no_advance_iter(bio)) {
>>> iter->bi_size -= bytes;
>>> - else
>>> + } else if (bio_flagged(bio, BIO_DMABUF_MAP)) {
>>> + bio_advance_iter_dmabuf_map(iter, bytes);
>>
>> This is a bit of a mess. You're using bi_bvec_done for something that
>> is not bvec_done, which makes the naming very confusing. That is even
>> more confusing than the existing usage, which isn't great. Also we
>> add yet another conditional to heavily inlined code. I'd suggest
>> the following:
>>
>> - add a prep patch to rename bi_bvec_done to bi_offset, as even for
>> the existing usage it is the offset into the current bio_vec as
>> much as it is the count of byes done, as those must be the same
>> and it is used both ways
>> - add a prep patch to also increase bi_offset for bio_no_advance_iter.
>> It is not actually use there, but incrementing it is harmless and
>> this will avoid a new special case
>> - please also documet this new usage in the commet in struct bvec_iter.
>> - then just add the dma buf mapping to the bio_no_advance_iter condition
>
> I'll take a look
>
>> - figure out what to do about dm_bio_rewind_iter, which pokes into these
>> things that really should be block layer internal
>
> Need to check what that is, but doesn't implement the interface and
> is not supposed to ever see the dmabuf iterator.
>
>>> }
>>> @@ -391,7 +403,7 @@ static inline void bio_wouldblock_error(struct bio *bio)
>>> */
>>> static inline int bio_iov_vecs_to_alloc(struct iov_iter *iter, int max_segs)
>>> {
>>> - if (iov_iter_is_bvec(iter))
>>> + if (iov_iter_is_bvec(iter) || iov_iter_is_dmabuf_map(iter))
>>> return 0;
>>> return iov_iter_npages(iter, max_segs);
>>> }
>>
>> Please update the comment for this helper.
>>
>>> @@ -322,6 +327,7 @@ enum {
>>> BIO_REMAPPED,
>>> BIO_ZONE_WRITE_PLUGGING, /* bio handled through zone write plugging */
>>> BIO_EMULATES_ZONE_APPEND, /* bio emulates a zone append operation */
>>> + BIO_DMABUF_MAP, /* Using premmaped dma buffers */
>>
>> Shouldn't this be a REQ_ flag as we should never mix and match bios with
>> and without this flag in a single request?
>
> Do you mean adding both and propagating it from bio to req? submit_bio()
> takes a bio, so we still need to set it there before it reaches blk-mq.
> And there might be bio-based drivers using it in the future.
>
^ permalink raw reply
* Re: [PATCH] iomap: don't make REQ_POLLED imply REQ_NOWAIT
From: Christian Brauner @ 2026-05-18 12:23 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Christian Brauner, djwong, axboe, linux-xfs, linux-fsdevel,
linux-block
In-Reply-To: <20260518062917.506483-1-hch@lst.de>
On Mon, 18 May 2026 08:29:13 +0200, Christoph Hellwig wrote:
> As described in commit 2bc057692599 ("block: don't make REQ_POLLED imply
> REQ_NOWAIT"), which fixed the same issue for the block device node, there
> are valid cases to poll for I/O completion without REQ_NOWAIT.
>
> Additionally, sing REQ_NOWAIT for file system writes is currently not
> supported as file systems writes are not idempotent and would need a
> retry of just the bio and not the entire operation to be fully supported.
>
> [...]
Applied to the vfs-7.2.iomap branch of the vfs/vfs.git tree.
Patches in the vfs-7.2.iomap branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-7.2.iomap
[1/1] iomap: don't make REQ_POLLED imply REQ_NOWAIT
https://git.kernel.org/vfs/vfs/c/47f28b493daf
^ permalink raw reply
* Re: [PATCH 1/2] direct-io: remove IOCB_NOWAIT support
From: Christian Brauner @ 2026-05-18 12:26 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Jens Axboe, Jan Kara, linux-fsdevel, linux-block
In-Reply-To: <20260518063336.507369-2-hch@lst.de>
On Mon, May 18, 2026 at 08:33:29AM +0200, Christoph Hellwig wrote:
> None of the file systems using the legacy direct I/O code actually sets
> FMODE_NOWAIT, and if they did this would not work, as the write locking
> could not handle the retry. Remove this dead code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
Reviewed-by: Christian Brauner <brauner@kernel.org>
^ permalink raw reply
* Re: [PATCH 2/2] block: don't set BIO_QUIET for BLK_STS_AGAIN
From: Jan Kara @ 2026-05-18 12:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Christian Brauner, Jan Kara, linux-fsdevel,
linux-block
In-Reply-To: <20260518063336.507369-3-hch@lst.de>
On Mon 18-05-26 08:33:30, Christoph Hellwig wrote:
> Commit abb30460bda2 ("block: mark bio_wouldblock_error() bio with
> BIO_QUIET") added this to suppress buffer_head warnings, but neither
> when this commit was added nor now any buffer_head using code actually
> ever sets REQ_NOWAIT which can lead to BLK_STS_AGAIN.
>
> Remove the special handling for now. If we ever plan to use REQ_NOWAIT
> for buffer_head based I/O we're better off handling BLK_STS_AGAIN in
> the completion handler as it actually needs to retry the I/O as well.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
So I agree about the bh argument but bio_wouldblock_error() is not specific
to bh path in any way, is it? So do we expect all bi_end_io handlers to
check for BLK_STS_AGAIN and avoid complaining to logs in case of that? Not
that all of them would be currently checking for BIO_QUIET either so I'm
mostly trying to figure out what's the expected handling.
Honza
> ---
> include/linux/bio.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 85463981d0f5..7597ae4dc52b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -379,7 +379,6 @@ static inline void bio_io_error(struct bio *bio)
>
> static inline void bio_wouldblock_error(struct bio *bio)
> {
> - bio_set_flag(bio, BIO_QUIET);
> bio->bi_status = BLK_STS_AGAIN;
> bio_endio(bio);
> }
> --
> 2.53.0
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Pavel Begunkov @ 2026-05-18 12:40 UTC (permalink / raw)
To: Christian König, Christoph Hellwig
Cc: Jens Axboe, Keith Busch, Sagi Grimberg, Alexander Viro,
Christian Brauner, Andrew Morton, Sumit Semwal, linux-block,
linux-kernel, linux-nvme, linux-fsdevel, io-uring, linux-media,
dri-devel, linaro-mm-sig, Nitesh Shetty, Kanchan Joshi,
Anuj Gupta, Tushar Gohad, William Power, Phil Cayton,
Jason Gunthorpe
In-Reply-To: <df697a76-c700-4908-ac08-a47ad07e0796@amd.com>
On 5/18/26 13:22, Christian König wrote:
> On 5/18/26 12:29, Pavel Begunkov wrote:
>> On 5/13/26 09:19, Christoph Hellwig wrote:
>>>> + if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
>>>> + bio->bi_io_vec = bio_src->bi_io_vec;
>>>> + } else {
>>>> + bio->dmabuf_map = bio_src->dmabuf_map;
>>>> + bio_set_flag(bio, BIO_DMABUF_MAP);
>>>> + }
>>>
>>> This is backwards, please avoid pointless negations:
>>
>> I can flip it, but compilers tend to prefer the true branch. E.g. this
>>
>> if (cond) A; else B;
>> C;
>>
>> can get compiled into:
>>
>> jmpcc cond B
>> A: ...
>> C:
>> return;
>> B: ...
>> jmp C;
>
> When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.
That might be more penalising than placing them in the right order,
and it might be fine as it's new and all that, but it's not a clear
cut as it's definitely not created to be a slow path. TBH, not sure
why we're bike shedding such things, is it somewhere in the code
style?
> What could be useful is to have the true path for the more common and the false path for the less common case for documentation purposes, but in that case I would expected some code comments as well.
What kind of comment are you thinking about? A "this is not a likely
path" type of comment before each mention of the flag is usually not
very useful.
--
Pavel Begunkov
^ permalink raw reply
* Re: [PATCH 2/2] block: don't set BIO_QUIET for BLK_STS_AGAIN
From: Christoph Hellwig @ 2026-05-18 12:51 UTC (permalink / raw)
To: Jan Kara
Cc: Christoph Hellwig, Jens Axboe, Christian Brauner, linux-fsdevel,
linux-block
In-Reply-To: <4j3y4zm2vsbyxlpmyktz4g3sbfwucw64h7p5t3hos3bvtinax4@j5dj3aeahnwa>
On Mon, May 18, 2026 at 02:34:49PM +0200, Jan Kara wrote:
> > Commit abb30460bda2 ("block: mark bio_wouldblock_error() bio with
> > BIO_QUIET") added this to suppress buffer_head warnings, but neither
> > when this commit was added nor now any buffer_head using code actually
> > ever sets REQ_NOWAIT which can lead to BLK_STS_AGAIN.
> >
> > Remove the special handling for now. If we ever plan to use REQ_NOWAIT
> > for buffer_head based I/O we're better off handling BLK_STS_AGAIN in
> > the completion handler as it actually needs to retry the I/O as well.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> So I agree about the bh argument but bio_wouldblock_error() is not specific
> to bh path in any way, is it? So do we expect all bi_end_io handlers to
> check for BLK_STS_AGAIN and avoid complaining to logs in case of that? Not
> that all of them would be currently checking for BIO_QUIET either so I'm
> mostly trying to figure out what's the expected handling.
File system would need special handling for BLK_STS_AGAIN were we to wire
it up again (in upstream we have the dead direct-io code removed in
patch 1, and an incorrect use in iomap polled I/O I sent a separate patch
for), because simply bubbling it up as -EAGAIN will generally do the
wrong thing. We ran into file system corruption last time we did it due
to non-idempotency of writes and general locking issues with just
retrying at a higher level.
Thus the rough consensus was that If we want to support fs-issued
REQ_NOWAIT I/O we'd first need to add infrastructure to retry them from
a blocking context at the bio-level, preferably in the block layer
itself.
^ permalink raw reply
* Re: [PATCH v3 05/10] lib: add dmabuf token infrastructure
From: Christoph Hellwig @ 2026-05-18 12:53 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Christoph Hellwig, Jens Axboe, Keith Busch, Sagi Grimberg,
Alexander Viro, Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <ebf41920-5852-428f-b98a-e0f44c8f3315@gmail.com>
On Mon, May 18, 2026 at 11:14:09AM +0100, Pavel Begunkov wrote:
>> This is about dma-buf based I/O. So I'd expect it to be named dma-buf-io
>> and no io-dmabuf, and live in drivers/dma-buf and not the unrelated lib/.
>> But I'd like to hear from the dma-buf maintainers about that.
>
> Looking at what Ming is saying, it'd make more sense to keep some of the
> parts like iterator and the file op more flexible and not automatically
> imply dma-buf even if it's the main and for now the only medium. I.e.
> ublk/fuse can use a similar interface for mapping buffers to the server
> even without dma mappings.
>
> I don't know how the API should look like, maybe passing memfd, and dma-buf
> supports mmap, but I think it's better to call the op something like
> "register_buffer" instead and keep all it in lib/ for the same reasons.
Let's get the current version landed. If we come up with some kind of
non-dma dmabuf in the future we can refactor it and move it around.
I'm a little skeptic we'll be able to share code as long as dmabuf
is allergic to physical addresses, though.
lib/ is most certainly the wrong place for something that absolutely
is not library functionality but directly interacts with a few
subsystems.
^ permalink raw reply
* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Christoph Hellwig @ 2026-05-18 12:54 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Christoph Hellwig, Jens Axboe, Keith Busch, Sagi Grimberg,
Alexander Viro, Christian Brauner, Andrew Morton, Sumit Semwal,
Christian König, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <24833f76-2289-4859-86d1-9215b11a1258@gmail.com>
On Mon, May 18, 2026 at 11:29:54AM +0100, Pavel Begunkov wrote:
> On 5/13/26 09:19, Christoph Hellwig wrote:
>>> + if (!bio_flagged(bio_src, BIO_DMABUF_MAP)) {
>>> + bio->bi_io_vec = bio_src->bi_io_vec;
>>> + } else {
>>> + bio->dmabuf_map = bio_src->dmabuf_map;
>>> + bio_set_flag(bio, BIO_DMABUF_MAP);
>>> + }
>>
>> This is backwards, please avoid pointless negations:
>
> I can flip it, but compilers tend to prefer the true branch. E.g. this
What kind of compiler optimization do you expect from code where
both sides of the conditional assign different arms of the same union
and just propagate the bit that was set?
^ permalink raw reply
* Re: [PATCH v3 04/10] block: introduce dma map backed bio type
From: Christoph Hellwig @ 2026-05-18 12:57 UTC (permalink / raw)
To: Pavel Begunkov
Cc: Christian König, Christoph Hellwig, Jens Axboe, Keith Busch,
Sagi Grimberg, Alexander Viro, Christian Brauner, Andrew Morton,
Sumit Semwal, linux-block, linux-kernel, linux-nvme,
linux-fsdevel, io-uring, linux-media, dri-devel, linaro-mm-sig,
Nitesh Shetty, Kanchan Joshi, Anuj Gupta, Tushar Gohad,
William Power, Phil Cayton, Jason Gunthorpe
In-Reply-To: <4561c621-817c-46be-8ff0-0b557f6c819d@gmail.com>
On Mon, May 18, 2026 at 01:40:18PM +0100, Pavel Begunkov wrote:
>> When that is really a performance critical path then you can use the likely() and unlikely() macros to give the compiler the hint which one to prefer.
>
> That might be more penalising than placing them in the right order,
> and it might be fine as it's new and all that, but it's not a clear
> cut as it's definitely not created to be a slow path.
Yes. Whatever the caller is using at a given time is the fast path here,
and dynamic branch prediction in modern cpus handles this really well.
> TBH, not sure
> why we're bike shedding such things, is it somewhere in the code
> style?
It makes reading the code annoying, so it better have a good reason.
Now for a single conditional it's not much of an issue, but these
things tend to pile up and then start to get really annoying.
Always write your code the most straight forward way unless you
have a good reason not to.
>> What could be useful is to have the true path for the more common and the false path for the less common case for documentation purposes, but in that case I would expected some code comments as well.
> What kind of comment are you thinking about? A "this is not a likely
> path" type of comment before each mention of the flag is usually not
> very useful.
Indeed. It's also not true here. If the workload uses dmabufs, the
path obviously is very likely.
^ permalink raw reply
* Re: [PATCH 2/2] block: don't set BIO_QUIET for BLK_STS_AGAIN
From: Jan Kara @ 2026-05-18 13:03 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jan Kara, Jens Axboe, Christian Brauner, linux-fsdevel,
linux-block
In-Reply-To: <20260518125109.GA5286@lst.de>
On Mon 18-05-26 14:51:09, Christoph Hellwig wrote:
> On Mon, May 18, 2026 at 02:34:49PM +0200, Jan Kara wrote:
> > > Commit abb30460bda2 ("block: mark bio_wouldblock_error() bio with
> > > BIO_QUIET") added this to suppress buffer_head warnings, but neither
> > > when this commit was added nor now any buffer_head using code actually
> > > ever sets REQ_NOWAIT which can lead to BLK_STS_AGAIN.
> > >
> > > Remove the special handling for now. If we ever plan to use REQ_NOWAIT
> > > for buffer_head based I/O we're better off handling BLK_STS_AGAIN in
> > > the completion handler as it actually needs to retry the I/O as well.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> >
> > So I agree about the bh argument but bio_wouldblock_error() is not specific
> > to bh path in any way, is it? So do we expect all bi_end_io handlers to
> > check for BLK_STS_AGAIN and avoid complaining to logs in case of that? Not
> > that all of them would be currently checking for BIO_QUIET either so I'm
> > mostly trying to figure out what's the expected handling.
>
> File system would need special handling for BLK_STS_AGAIN were we to wire
> it up again (in upstream we have the dead direct-io code removed in
> patch 1, and an incorrect use in iomap polled I/O I sent a separate patch
> for), because simply bubbling it up as -EAGAIN will generally do the
> wrong thing. We ran into file system corruption last time we did it due
> to non-idempotency of writes and general locking issues with just
> retrying at a higher level.
>
> Thus the rough consensus was that If we want to support fs-issued
> REQ_NOWAIT I/O we'd first need to add infrastructure to retry them from
> a blocking context at the bio-level, preferably in the block layer
> itself.
OK, fair. Thanks for explanation. So this flagging with quiet is indeed
pointless. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply
* [bug report] drbd: allow parallel promote/demote actions
From: Dan Carpenter @ 2026-05-18 13:10 UTC (permalink / raw)
To: Lars Ellenberg; +Cc: drbd-dev, linux-block
[ This code is 12 years old. It's weird to me that no one has complained
about this bug in real life but I suppose that unless you have kfree
poisoning or KASAN enabled then probably you wouldn't notice it. ]
Hello Lars Ellenberg,
Commit 9e276872fe16 ("drbd: allow parallel promote/demote actions")
from Apr 28, 2014 (linux-next), leads to the following Smatch static
checker warning:
drivers/block/drbd/drbd_nl.c:4564 drbd_adm_down() error: dereferencing freed memory 'resource' (line 4562)
drivers/block/drbd/drbd_nl.c:4585 drbd_adm_del_resource() error: dereferencing freed memory 'resource' (line 4584)
drivers/block/drbd/drbd_nl.c
4552 /* delete volumes */
4553 idr_for_each_entry(&resource->devices, device, i) {
4554 retcode = adm_del_minor(device);
4555 if (retcode != NO_ERROR) {
4556 /* "can not happen" */
4557 drbd_msg_put_info(adm_ctx->reply_skb, "failed to delete volume");
4558 goto out;
4559 }
4560 }
4561
4562 retcode = adm_del_resource(resource);
^^^^^^^^^^^^^^^^^^^^^^^^^^
This frees "resource"
4563 out:
--> 4564 mutex_unlock(&resource->adm_mutex);
^^^^^^^^^
Use after free.
4565 finish:
4566 adm_ctx->reply_dh->ret_code = retcode;
4567 return 0;
4568 }
This email is a free service from the Smatch-CI project [smatch.sf.net].
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH v6 0/2] blk-mq: introduce tag starvation observability
From: Jens Axboe @ 2026-05-18 13:31 UTC (permalink / raw)
To: Aaron Tomlin, rostedt, mhiramat, mathieu.desnoyers
Cc: bvanassche, johannes.thumshirn, kch, dlemoal, ritesh.list,
loberman, neelx, sean, mproche, chjohnst, linux-block,
linux-kernel, linux-trace-kernel
In-Reply-To: <20260517213614.350367-1-atomlin@atomlin.com>
On 5/17/26 3:36 PM, Aaron Tomlin wrote:
> Hi Jens, Steve, Masami,
>
> In high-performance storage environments, particularly when utilising RAID
> controllers with shared tag sets (BLK_MQ_F_TAG_HCTX_SHARED), severe latency
> spikes can occur when fast devices are starved of available tags.
> Currently, diagnosing this specific queue contention requires deploying
> dynamic kprobes or inferring sleep states, which lacks a simple,
> out-of-the-box diagnostic path.
>
> This short series introduces dedicated, low-overhead observability for tag
> exhaustion events in the block layer:
>
> - Patch 1 introduces the "block_rq_tag_wait" tracepoint in the tag
> allocation slow-path to capture precise, event-based starvation.
>
> - Patch 2 complements this by exposing "wait_on_hw_tag" and
> "wait_on_sched_tag" per-CPU counters via debugfs for quick,
> point-in-time cumulative polling.
>
> Together, these provide storage engineers with zero-configuration
> mechanisms to definitively identify shared-tag bottlenecks.
Why not just issue the trace points? Then there's close to zero
overhead, rather than needing to need added counters for this, and the
kernel to keep track. If you just issue the get/put tag kind of traces,
then userspace can keep track. That's what blktrace has done for decades
for things like inflight/queue depth accounting.
IOW, seems to me, this could be done with basically zero kernel
additions outside of perhaps a trace point or two.
--
Jens Axboe
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox