On 12/4/25 10:56, Christoph Hellwig wrote:
On Sun, Nov 23, 2025 at 10:51:25PM +0000, Pavel Begunkov wrote:
...
+ struct request_queue *q = bdev_get_queue(file_bdev(file));
+
+ if (!(file->f_flags & O_DIRECT))
+ return ERR_PTR(-EINVAL);
Shouldn't the O_DIRECT check be in the caller?
If the interface will get implemented e.g. for net at some point, it
won't be O_DIRECT. If you want some extra safety for fs implementing
it, I can add sth like below in the common path:
if (reg_or_block_file(file))
// check O_DIRECT
And a high-level comment explaining the fencing logic would be nice
as well.
I'll add some comments around
...
+static inline
+struct blk_mq_dma_map *blk_mq_get_token_map(struct blk_mq_dma_token *token)
Really odd return value / scope formatting.
static inline struct blk_mq_dma_map
*blk_mq_get_token_map(...)
Do you prefer this? It's too long to sanely fit it in
either way. Though I didn't have this problem in v3.
+{
+ struct blk_mq_dma_map *map;
+
+ guard(rcu)();
+
+ map = rcu_dereference(token->map);
+ if (unlikely(!map || !percpu_ref_tryget_live_rcu(&map->refs)))
+ return NULL;
+ return map;
Please use good old rcu_read_unlock to make this readable.
Come on, it's pretty readable and less error prone, especially
for longer functions. Maybe you prefer scoped guards?
scoped_guard(rcu) {
map = token->map;
if (!map)
return;
}
...
+blk_status_t blk_rq_assign_dma_map(struct request *rq,
+ struct blk_mq_dma_token *token)
+{
+ struct blk_mq_dma_map *map;
+
+ map = blk_mq_get_token_map(token);
+ if (map)
+ goto complete;
+
+ if (rq->cmd_flags & REQ_NOWAIT)
+ return BLK_STS_AGAIN;
+
+ map = blk_mq_create_dma_map(token);
+ if (IS_ERR(map))
+ return BLK_STS_RESOURCE;
Having a few comments, that say this is creating the map lazily
would probably helper the reader. Also why not keep the !map
case in the branch, as the map case should be the fast path and
thus usually be straight line in the function?
+void blk_mq_dma_map_move_notify(struct blk_mq_dma_token *token)
+{
+ blk_mq_dma_map_remove(token);
+}
Is there a good reason for having this blk_mq_dma_map_move_notify
wrapper?
I was reused it before and reusing in the next iteration, maybe
v2 wasn't for some reason.
+ if (bio_flagged(bio, BIO_DMA_TOKEN)) {
+ struct blk_mq_dma_token *token;
+ blk_status_t ret;
+
+ token = dma_token_to_blk_mq(bio->dma_token);
+ ret = blk_rq_assign_dma_map(rq, token);
+ if (ret) {
+ if (ret == BLK_STS_AGAIN) {
+ bio_wouldblock_error(bio);
+ } else {
+ bio->bi_status = BLK_STS_RESOURCE;
+ bio_endio(bio);
+ }
+ goto queue_exit;
+ }
+ }
Any reason to not just keep the dma_token_to_blk_mq? Also why is this
overriding non-BLK_STS_AGAIN errors with BLK_STS_RESOURCE?
Yeah, it should've been errno_to_blk_status()
--
Pavel Begunkov
