Re: [PATCH 2/2] block: add BLKSETDESCZONE ioctl for Zoned Block Devices
From: Bart Van Assche <bvanassche@acm.org>
Date: 2020-06-29 01:36:31
Also in:
linux-nvme, linux-scsi, lkml
On 2020-06-28 16:01, Matias Bjørling wrote:
+ /* This may take a while, so be nice to others */ + cond_resched(); + + return submit_bio_wait(&bio);
A cond_resched() call before a submit_bio_wait() call? I think it's the first time that I see this. Is that call really necessary? Isn't the wait_for_completion() call inside submit_bio_wait() sufficient?
+ /* no flags is currently supported */
^^
are?
+ /* allocate the size of the zone descriptor extension and fill
+ * with the data in the user data buffer. If the data size is less
+ * than the zone descriptor extension size, then the rest of the
+ * zone description extension data buffer is zero-filled.
+ */
+ zsd_data = (void *) get_zeroed_page(GFP_KERNEL);
+ if (!zsd_data)
+ return -ENOMEM;
+
+ if (copy_from_user(zsd_data, argp + sizeof(struct blk_zone_set_desc),
+ zsd.len)) {
+ ret = -EFAULT;
+ goto free;
+ }Has it been considered to use kmalloc() instead of get_zeroed_page()?
quoted hunk ↗ jump to hunk
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h index ccb895f911b1..53b7b05b0004 100644 --- a/include/linux/blk_types.h +++ b/include/linux/blk_types.h@@ -316,6 +316,8 @@ enum req_opf { REQ_OP_ZONE_FINISH = 12, /* write data at the current zone write pointer */ REQ_OP_ZONE_APPEND = 13, + /* associate zone desc extension data to a zone */ + REQ_OP_ZONE_SET_DESC = 14, /* SCSI passthrough using struct scsi_request */ REQ_OP_SCSI_IN = 32,
Does REQ_OP_ZONE_SET_DESC count as a read or as a write operation? See also:
static inline bool op_is_write(unsigned int op)
{
return (op & 1);
}
+/**
+ * struct blk_zone_set_desc - BLKSETDESCZONE ioctl requests
+ * @sector: Starting sector of the zone to operate on.
+ * @flags: Feature flags.
+ * @len: size, in bytes, of the data to be associated to the zone.
+ * @data: data to be associated.
+ */
+struct blk_zone_set_desc {
+ __u64 sector;
+ __u32 flags;
+ __u32 len;
+ __u8 data[0];
+};Isn't the recommended style to use a flexible array ([] instead of [0])? See also https://lore.kernel.org/lkml/20200608213711.GA22271@embeddedor/ (local). Thanks, Bart.