[PATCH v10] nvmet: add cgroup_id to charge namespace I/O to a cgroup
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon Oct 26 08:53:47 PDT 2026
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] nvmet: secondary blkcg_wq works miss memalloc_noio_save() and sq dying check
--
commit 13b26415d838fcd912a4355b04d128dd0c4fb228
Author: Peng Yu <yupeng0921 at gmail.com>
nvmet: add cgroup_id to charge namespace I/O to a cgroup
This commit adds a configfs cgroup_id attribute to nvmet namespaces so
that namespace I/O can be associated with a blkcg css. To prevent
blocking shared transport threads when io.cost or io.latency throttles a
thread, it executes I/O commands for cgroup-associated namespaces via a
per-namespace workqueue.
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 4955872dc1fc2..624955cc7fb55 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
[ ... ]
> @@ -474,8 +477,144 @@ void nvmet_put_namespace(struct nvmet_ns *ns)
[ ... ]
> +static void nvmet_blkcg_work(struct work_struct *work)
> +{
> + struct nvmet_req *req =
> + container_of(work, struct nvmet_req, blkcg_work);
> + unsigned int noio_flag;
> + bool associated;
> +
> + /* its queue is being deleted: abort it rather than throttle it */
> + if (unlikely(percpu_ref_is_dying(&req->sq->ref))) {
> + nvmet_req_complete(req, NVME_SC_ABORT_QUEUE);
> + return;
> + }
> +
> + /* reclaim must not wait for I/O queued on this workqueue */
> + noio_flag = memalloc_noio_save();
> + associated = nvmet_blkcg_begin(req);
> + req->blkcg_execute(req);
> + nvmet_blkcg_end(associated);
> + memalloc_noio_restore(noio_flag);
> +}
> +
> +static void nvmet_blkcg_queue_req(struct nvmet_req *req)
> +{
> + INIT_WORK(&req->blkcg_work, nvmet_blkcg_work);
> + queue_work(req->ns->blkcg_wq, &req->blkcg_work);
> +}
> +
> +static void nvmet_blkcg_init_req(struct nvmet_req *req)
> +{
> + if (!req->ns->cgrp)
> + return;
> +
> + req->blkcg_execute = req->execute;
> + req->execute = nvmet_blkcg_queue_req;
> +}
[ ... ]
> diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
> index 695963dd932ee..673df713c954f 100644
> --- a/drivers/nvme/target/io-cmd-file.c
> +++ b/drivers/nvme/target/io-cmd-file.c
> @@ -257,8 +257,13 @@ u16 nvmet_file_flush(struct nvmet_req *req)
> static void nvmet_file_flush_work(struct work_struct *w)
> {
> struct nvmet_req *req = container_of(w, struct nvmet_req, f.work);
> + bool associated;
> + u16 status;
>
> - nvmet_req_complete(req, nvmet_file_flush(req));
> + associated = nvmet_blkcg_begin(req);
> + status = nvmet_file_flush(req);
> + nvmet_blkcg_end(associated);
> + nvmet_req_complete(req, status);
> }
>
> static void nvmet_file_execute_flush(struct nvmet_req *req)
> @@ -266,7 +271,7 @@ static void nvmet_file_execute_flush(struct nvmet_req *req)
> if (!nvmet_check_transfer_len(req, 0))
> return;
> INIT_WORK(&req->f.work, nvmet_file_flush_work);
> - queue_work(nvmet_wq, &req->f.work);
> + nvmet_req_queue_work(req, nvmet_wq, &req->f.work);
> }
>
> static void nvmet_file_execute_discard(struct nvmet_req *req)
> @@ -274,10 +279,12 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
> int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
> struct nvme_dsm_range range;
> loff_t offset, len;
> + bool associated;
> u16 status = 0;
> int ret;
> int i;
>
> + associated = nvmet_blkcg_begin(req);
[ ... ]
> @@ -326,7 +334,7 @@ static void nvmet_file_execute_dsm(struct nvmet_req *req)
> if (!nvmet_check_data_len_lte(req, nvmet_dsm_len(req)))
> return;
> INIT_WORK(&req->f.work, nvmet_file_dsm_work);
> - queue_work(nvmet_wq, &req->f.work);
> + nvmet_req_queue_work(req, nvmet_wq, &req->f.work);
> }
>
> static void nvmet_file_write_zeroes_work(struct work_struct *w)
> @@ -336,6 +344,7 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
> int mode = FALLOC_FL_ZERO_RANGE | FALLOC_FL_KEEP_SIZE;
> loff_t offset;
> loff_t len;
> + bool associated;
> int ret;
[ ... ]
> @@ -347,7 +356,9 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
> return;
> }
>
> + associated = nvmet_blkcg_begin(req);
> ret = vfs_fallocate(req->ns->file, mode, offset, len);
> + nvmet_blkcg_end(associated);
> nvmet_req_complete(req, ret < 0 ? errno_to_nvme_status(req, ret) : 0);
> }
>
> @@ -356,7 +367,7 @@ static void nvmet_file_execute_write_zeroes(struct nvmet_req *req)
> if (!nvmet_check_transfer_len(req, 0))
> return;
> INIT_WORK(&req->f.work, nvmet_file_write_zeroes_work);
> - queue_work(nvmet_wq, &req->f.work);
> + nvmet_req_queue_work(req, nvmet_wq, &req->f.work);
> }
[ ... ]
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index e315f1c24e73d..9a9fba0512485 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
[ ... ]
> @@ -746,6 +767,36 @@ void nvmet_bdev_execute_zone_mgmt_recv(struct nvmet_req *req);
[ ... ]
> +static inline void nvmet_req_queue_work(struct nvmet_req *req,
> + struct workqueue_struct *wq, struct work_struct *work)
> +{
> +#ifdef CONFIG_BLK_CGROUP
> + if (req->ns->blkcg_wq)
> + wq = req->ns->blkcg_wq;
> +#endif
> + queue_work(wq, work);
> +}
> +
> static inline u32 nvmet_rw_data_len(struct nvmet_req *req)
> diff --git a/drivers/nvme/target/zns.c b/drivers/nvme/target/zns.c
> index 92f5c1435582e..06d9a12245719 100644
> --- a/drivers/nvme/target/zns.c
> +++ b/drivers/nvme/target/zns.c
> @@ -498,8 +498,11 @@ static void nvmet_bdev_zmgmt_send_work(struct work_struct *w)
> struct block_device *bdev = req->ns->bdev;
> sector_t zone_sectors = bdev_zone_sectors(bdev);
> u16 status = NVME_SC_SUCCESS;
> + bool associated;
> int ret;
>
> + associated = nvmet_blkcg_begin(req);
> +
> if (op == REQ_OP_LAST) {
> req->error_loc = offsetof(struct nvme_zone_mgmt_send_cmd, zsa);
> status = NVME_SC_ZONE_INVALID_TRANSITION | NVME_STATUS_DNR;
> @@ -529,13 +532,14 @@ static void nvmet_bdev_zmgmt_send_work(struct work_struct *w)
> status = blkdev_zone_mgmt_errno_to_nvme_status(ret);
>
> out:
> + nvmet_blkcg_end(associated);
> nvmet_req_complete(req, status);
> }
>
> void nvmet_bdev_execute_zone_mgmt_send(struct nvmet_req *req)
> {
> INIT_WORK(&req->z.zmgmt_work, nvmet_bdev_zmgmt_send_work);
> - queue_work(zbd_wq, &req->z.zmgmt_work);
> + nvmet_req_queue_work(req, zbd_wq, &req->z.zmgmt_work);
> }
[Severity: High]
When a namespace has cgroup_id configured, nvmet_parse_io_cmd() calls
nvmet_blkcg_init_req() in drivers/nvme/target/core.c for all I/O commands,
wrapping req->execute with nvmet_blkcg_queue_req().
For commands whose backend execute callbacks queue another work item via
nvmet_req_queue_work() (nvmet_file_execute_flush(), nvmet_file_execute_dsm(),
nvmet_file_execute_write_zeroes() in drivers/nvme/target/io-cmd-file.c, and
nvmet_bdev_execute_zone_mgmt_send() in drivers/nvme/target/zns.c), the call
chain becomes:
nvmet_blkcg_queue_req()
-> queue_work(req->ns->blkcg_wq, &req->blkcg_work)
nvmet_blkcg_work()
-> percpu_ref_is_dying(&req->sq->ref)
-> memalloc_noio_save()
-> nvmet_blkcg_begin()
-> req->blkcg_execute(req)
-> nvmet_req_queue_work(req, ..., work)
-> queue_work(req->ns->blkcg_wq, work)
-> nvmet_blkcg_end()
-> memalloc_noio_restore()
Because req->blkcg_execute(req) only queues a second work item onto
req->ns->blkcg_wq and returns immediately, nvmet_blkcg_work() restores
noio_flag before the actual I/O work runs.
When the second work item (nvmet_file_flush_work(), nvmet_file_dsm_work() ->
nvmet_file_execute_discard(), nvmet_file_write_zeroes_work(), or
nvmet_bdev_zmgmt_send_work()) later executes on req->ns->blkcg_wq:
1. It runs without memalloc_noio_save() on a WQ_MEM_RECLAIM workqueue while
performing GFP_KERNEL / __GFP_IO allocations (such as blk_next_bio() with
GFP_KERNEL in blkdev_zone_mgmt() and nvmet_bdev_zone_mgmt_emulate_all(),
blkdev_report_zones(), or filesystem allocations in vfs_fallocate() and
vfs_fsync()). Can direct reclaim from these allocations recurse into I/O and
deadlock waiting for requests queued behind throttled workers on the same
req->ns->blkcg_wq?
2. It does not check percpu_ref_is_dying(&req->sq->ref) before calling
nvmet_blkcg_begin(req). If nvmet_sq_destroy() kills sq->ref while this second
work item is waiting behind throttled commands on req->ns->blkcg_wq, won't it
still enter cgroup throttling instead of aborting early with
NVME_SC_ABORT_QUEUE?
Would it make sense for these commands to either execute directly in
nvmet_blkcg_work() without queuing a second work item onto req->ns->blkcg_wq,
or skip nvmet_blkcg_init_req() and apply the dying-queue check and
memalloc_noio_save() inside their own work handlers?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261026153020.47791-1-yupeng0921@gmail.com?part=1
More information about the Linux-nvme
mailing list