[PATCH 0/7] scsi: target: Add WRITE_ATOMIC_16 support

[PATCH 0/7] scsi: target: Add WRITE_ATOMIC_16 support

[Mike Christie]

The following patches were made over Linus's current tree and add
WRITE_ATOMIC_16 support to LIO.

In this patchset we only support target_core_iblock. It's implemented
similar to UNMAP where we do not do any emulation and instead pass the
operation to the block layer.

[PATCH 1/7] scsi: target: Rename target_configure_unmap_from_queue

[Mike Christie]

Rename target_configure_unmap_from_queue to
target_configure_unmap_from_bdev since it now takes a bdev.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_device.c  | 6 +++---
 drivers/target/target_core_file.c    | 4 ++--
 drivers/target/target_core_iblock.c  | 4 ++--
 include/target/target_core_backend.h | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 7d43d92c44d4..b709a64c0ff3 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -797,8 +797,8 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
  * Check if the underlying struct block_device supports discard and if yes
  * configure the UNMAP parameters.
  */
-bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
-				       struct block_device *bdev)
+bool target_configure_unmap_from_bdev(struct se_dev_attrib *attrib,
+				      struct block_device *bdev)
 {
 	int block_size = bdev_logical_block_size(bdev);
 
@@ -816,7 +816,7 @@ bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
 		bdev_discard_alignment(bdev) / block_size;
 	return true;
 }
-EXPORT_SYMBOL(target_configure_unmap_from_queue);
+EXPORT_SYMBOL(target_configure_unmap_from_bdev);
 
 /*
  * Convert from blocksize advertised to the initiator to the 512 byte
diff --git a/drivers/target/target_core_file.c b/drivers/target/target_core_file.c
index 2d78ef74633c..b2610073e8cc 100644
--- a/drivers/target/target_core_file.c
+++ b/drivers/target/target_core_file.c
@@ -92,8 +92,8 @@ static bool fd_configure_unmap(struct se_device *dev)
 	struct inode *inode = file->f_mapping->host;
 
 	if (S_ISBLK(inode->i_mode))
-		return target_configure_unmap_from_queue(&dev->dev_attrib,
-							 I_BDEV(inode));
+		return target_configure_unmap_from_bdev(&dev->dev_attrib,
+							I_BDEV(inode));
 
 	/* Limit UNMAP emulation to 8k Number of LBAs (NoLB) */
 	dev->dev_attrib.max_unmap_lba_count = 0x2000;
diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index c8dc92a7d63e..3a191bc35e04 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -83,8 +83,8 @@ static bool iblock_configure_unmap(struct se_device *dev)
 {
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
 
-	return target_configure_unmap_from_queue(&dev->dev_attrib,
-						 ib_dev->ibd_bd);
+	return target_configure_unmap_from_bdev(&dev->dev_attrib,
+						ib_dev->ibd_bd);
 }
 
 static int iblock_configure_device(struct se_device *dev)
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 4063a701081b..d394306f8f49 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -121,8 +121,8 @@ sense_reason_t passthrough_parse_cdb(struct se_cmd *cmd,
 
 bool target_sense_desc_format(struct se_device *dev);
 sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
-bool target_configure_unmap_from_queue(struct se_dev_attrib *attrib,
-				       struct block_device *bdev);
+bool target_configure_unmap_from_bdev(struct se_dev_attrib *attrib,
+				      struct block_device *bdev);
 
 static inline bool target_dev_configured(struct se_device *se_dev)
 {
-- 
2.34.1

[PATCH 2/7] scsi: target: Add atomic se_device fields

[Mike Christie]

This adds atomic fields to the se_device and exports them in configfs.

Initially only target_core_iblock will be supported and we will inherit
all the settings from the block layer except the alignment which userspace
will set.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_configfs.c | 42 +++++++++++++++++++++++++++
 include/target/target_core_base.h     |  6 ++++
 2 files changed, 48 insertions(+)

diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
index c40217f44b1b..f3c7da650f65 100644
--- a/drivers/target/target_core_configfs.c
+++ b/drivers/target/target_core_configfs.c
@@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
 DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
 DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
 DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
+DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
 
 #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)				\
 static ssize_t _name##_store(struct config_item *item, const char *page,\
@@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct config_item *item, const char *page,
 	return count;
 }
 
+static ssize_t atomic_alignment_store(struct config_item *item,
+				      const char *page, size_t count)
+{
+	struct se_dev_attrib *da = to_attrib(item);
+	u32 val;
+	int ret;
+
+	ret = kstrtou32(page, 0, &val);
+	if (ret < 0)
+		return ret;
+
+	if (da->da_dev->export_count) {
+		pr_err("dev[%p]: Unable to change SE Device atomic_alignment while export_count is %d\n",
+		       da->da_dev, da->da_dev->export_count);
+		return -EINVAL;
+	}
+
+	da->atomic_alignment = val;
+
+	pr_debug("dev[%p]: SE Device atomic_alignment changed to %u\n",
+		 da->da_dev, val);
+	return count;
+}
+
 CONFIGFS_ATTR(, emulate_model_alias);
 CONFIGFS_ATTR(, emulate_dpo);
 CONFIGFS_ATTR(, emulate_fua_write);
@@ -1302,6 +1332,12 @@ CONFIGFS_ATTR(, max_write_same_len);
 CONFIGFS_ATTR(, alua_support);
 CONFIGFS_ATTR(, pgr_support);
 CONFIGFS_ATTR(, submit_type);
+CONFIGFS_ATTR(, atomic_alignment);
+CONFIGFS_ATTR_RO(, atomic_supported);
+CONFIGFS_ATTR_RO(, atomic_max_len);
+CONFIGFS_ATTR_RO(, atomic_granularity);
+CONFIGFS_ATTR_RO(, atomic_max_with_boundary);
+CONFIGFS_ATTR_RO(, atomic_max_boundary);
 
 /*
  * dev_attrib attributes for devices using the target core SBC/SPC
@@ -1345,6 +1381,12 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
 	&attr_pgr_support,
 	&attr_emulate_rsoc,
 	&attr_submit_type,
+	&attr_atomic_supported,
+	&attr_atomic_alignment,
+	&attr_atomic_max_len,
+	&attr_atomic_granularity,
+	&attr_atomic_max_with_boundary,
+	&attr_atomic_max_boundary,
 	NULL,
 };
 EXPORT_SYMBOL(sbc_attrib_attrs);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 97099a5e3f6c..6c87bd018983 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -715,6 +715,7 @@ struct se_dev_attrib {
 	bool		is_nonrot;
 	bool		emulate_rest_reord;
 	bool		unmap_zeroes_data;
+	bool		atomic_supported;
 	u32		hw_block_size;
 	u32		block_size;
 	u32		hw_max_sectors;
@@ -726,6 +727,11 @@ struct se_dev_attrib {
 	u32		unmap_granularity;
 	u32		unmap_granularity_alignment;
 	u32		max_write_same_len;
+	u32		atomic_max_len;
+	u32		atomic_alignment;
+	u32		atomic_granularity;
+	u32		atomic_max_with_boundary;
+	u32		atomic_max_boundary;
 	u8		submit_type;
 	struct se_device *da_dev;
 	struct config_group da_group;
-- 
2.34.1

[PATCH 3/7] scsi: target: Add helper to setup atomic values from block_device

[Mike Christie]

This adds a helper function that sets up the atomic value based on a
block_device similar to what we do for unmap.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_device.c  | 18 ++++++++++++++++++
 include/target/target_core_backend.h |  2 ++
 2 files changed, 20 insertions(+)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index b709a64c0ff3..4741de3d1061 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -793,6 +793,24 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 	return dev;
 }
 
+void target_configure_write_atomic_from_bdev(struct se_dev_attrib *attrib,
+					     struct block_device *bdev)
+{
+	struct request_queue *q = bdev_get_queue(bdev);
+	int block_size = bdev_logical_block_size(bdev);
+
+	if (!bdev_can_atomic_write(bdev))
+		return;
+
+	attrib->atomic_supported = true;
+	attrib->atomic_max_len = queue_atomic_write_max_bytes(q) / block_size;
+	attrib->atomic_granularity = queue_atomic_write_unit_min_bytes(q) /
+								block_size;
+	attrib->atomic_max_with_boundary = 0;
+	attrib->atomic_max_boundary = 0;
+}
+EXPORT_SYMBOL_GPL(target_configure_write_atomic_from_bdev);
+
 /*
  * Check if the underlying struct block_device supports discard and if yes
  * configure the UNMAP parameters.
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index d394306f8f49..e32de80854b6 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -123,6 +123,8 @@ bool target_sense_desc_format(struct se_device *dev);
 sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
 bool target_configure_unmap_from_bdev(struct se_dev_attrib *attrib,
 				      struct block_device *bdev);
+void target_configure_write_atomic_from_bdev(struct se_dev_attrib *attrib,
+					     struct block_device *bdev);
 
 static inline bool target_dev_configured(struct se_device *se_dev)
 {
-- 
2.34.1

[PATCH 4/7] scsi: target: Add WRITE_ATOMIC_16 handler

[Mike Christie]

This adds the core LIO code to process the WRITE_ATOMIC_16 command.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_sbc.c  | 48 +++++++++++++++++++++++++++++++
 include/target/target_core_base.h |  1 +
 2 files changed, 49 insertions(+)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index fe8beb7dbab1..0ae0d4ec0ac3 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -764,6 +764,46 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
 	return 0;
 }
 
+static int
+sbc_check_atomic(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
+{
+	struct se_dev_attrib *attrib = &dev->dev_attrib;
+	u16 boundary, transfer_len;
+	u32 lba;
+
+	if (!attrib->atomic_supported)
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+
+	lba = transport_lba_32(cdb);
+	boundary = get_unaligned_be16(&cdb[10]);
+	transfer_len = get_unaligned_be16(&cdb[12]);
+
+	if (!boundary) {
+		if (transfer_len > attrib->atomic_max_len)
+			return TCM_INVALID_CDB_FIELD;
+	} else {
+		if (transfer_len > attrib->atomic_max_with_boundary)
+			return TCM_INVALID_CDB_FIELD;
+
+		if (boundary > attrib->atomic_max_boundary)
+			return TCM_INVALID_CDB_FIELD;
+	}
+
+	if (attrib->atomic_granularity) {
+		if (transfer_len % attrib->atomic_granularity)
+			return TCM_INVALID_CDB_FIELD;
+
+		if (boundary && boundary % attrib->atomic_granularity)
+			return TCM_INVALID_CDB_FIELD;
+	}
+
+	if (dev->dev_attrib.atomic_alignment &&
+	    lba % dev->dev_attrib.atomic_alignment)
+		return TCM_INVALID_CDB_FIELD;
+
+	return 0;
+}
+
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
 {
@@ -861,6 +901,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
 		break;
 	case WRITE_16:
 	case WRITE_VERIFY_16:
+	case WRITE_ATOMIC_16:
 		sectors = transport_get_sectors_16(cdb);
 		cmd->t_task_lba = transport_lba_64(cdb);
 
@@ -872,6 +913,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
 			return ret;
 
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
+		if (cdb[0] == WRITE_ATOMIC_16) {
+			cmd->se_cmd_flags |= SCF_ATOMIC;
+
+			ret = sbc_check_atomic(dev, cmd, cdb);
+			if (ret)
+				return ret;
+		}
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case VARIABLE_LENGTH_CMD:
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 6c87bd018983..7b8ed21bea03 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -157,6 +157,7 @@ enum se_cmd_flags_table {
 	SCF_USE_CPUID				= (1 << 16),
 	SCF_TASK_ATTR_SET			= (1 << 17),
 	SCF_TREAT_READ_AS_NORMAL		= (1 << 18),
+	SCF_ATOMIC				= (1 << 19),
 };
 
 /*
-- 
2.34.1

[PATCH 5/7] scsi: target: Report atomic values in INQUIRY

[Mike Christie]

This reports the atomic values in the Block Limits VPD page.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_spc.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index ea14a3835681..ce5eec5c8b8a 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -521,7 +521,6 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
 		have_tp = 1;
 
 	buf[0] = dev->transport->get_device_type(dev);
-	buf[3] = have_tp ? 0x3c : 0x10;
 
 	/* Set WSNZ to 1 */
 	buf[4] = 0x01;
@@ -562,11 +561,10 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
 	else
 		put_unaligned_be32(dev->dev_attrib.optimal_sectors, &buf[12]);
 
-	/*
-	 * Exit now if we don't support TP.
-	 */
+	put_unaligned_be16(12, &buf[2]);
+
 	if (!have_tp)
-		goto max_write_same;
+		goto try_atomic;
 
 	/*
 	 * Set MAXIMUM UNMAP LBA COUNT
@@ -595,9 +593,29 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
 	/*
 	 * MAXIMUM WRITE SAME LENGTH
 	 */
-max_write_same:
 	put_unaligned_be64(dev->dev_attrib.max_write_same_len, &buf[36]);
 
+	put_unaligned_be16(40, &buf[2]);
+
+try_atomic:
+	/*
+	 * ATOMIC
+	 */
+	if (!dev->dev_attrib.atomic_supported)
+		goto done;
+
+	if (dev->dev_attrib.atomic_max_len < io_max_blocks)
+		put_unaligned_be32(dev->dev_attrib.atomic_max_len, &buf[44]);
+	else
+		put_unaligned_be32(io_max_blocks, &buf[44]);
+
+	put_unaligned_be32(dev->dev_attrib.atomic_alignment, &buf[48]);
+	put_unaligned_be32(dev->dev_attrib.atomic_granularity, &buf[52]);
+	put_unaligned_be32(dev->dev_attrib.atomic_max_with_boundary, &buf[56]);
+	put_unaligned_be32(dev->dev_attrib.atomic_max_boundary, &buf[60]);
+
+	put_unaligned_be16(60, &buf[2]);
+done:
 	return 0;
 }
 
-- 
2.34.1

[PATCH 6/7] scsi: target: Add WRITE_ATOMIC_16 support to RSOC

[Mike Christie]

This has us report if the device supports WRITE_ATOMIC_16 in the
REPORT_SUPPORTED_OPERATION_CODES command.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_spc.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/target/target_core_spc.c b/drivers/target/target_core_spc.c
index ce5eec5c8b8a..3c28b0ec5daf 100644
--- a/drivers/target/target_core_spc.c
+++ b/drivers/target/target_core_spc.c
@@ -1470,6 +1470,24 @@ static struct target_opcode_descriptor tcm_opcode_write_same32 = {
 	.update_usage_bits = set_dpofua_usage_bits32,
 };
 
+static bool tcm_is_atomic_enabled(struct target_opcode_descriptor *descr,
+				  struct se_cmd *cmd)
+{
+	return cmd->se_dev->dev_attrib.atomic_supported;
+}
+
+static struct target_opcode_descriptor tcm_opcode_write_atomic16 = {
+	.support = SCSI_SUPPORT_FULL,
+	.opcode = WRITE_ATOMIC_16,
+	.cdb_size = 16,
+	.usage_bits = {WRITE_ATOMIC_16, 0xf8, 0xff, 0xff,
+		       0xff, 0xff, 0xff, 0xff,
+		       0xff, 0xff, 0xff, 0xff,
+		       0xff, 0xff, SCSI_GROUP_NUMBER_MASK, SCSI_CONTROL_MASK},
+	.enabled = tcm_is_atomic_enabled,
+	.update_usage_bits = set_dpofua_usage_bits,
+};
+
 static bool tcm_is_caw_enabled(struct target_opcode_descriptor *descr,
 			       struct se_cmd *cmd)
 {
@@ -2026,6 +2044,7 @@ static struct target_opcode_descriptor *tcm_supported_opcodes[] = {
 	&tcm_opcode_write16,
 	&tcm_opcode_write_verify16,
 	&tcm_opcode_write_same32,
+	&tcm_opcode_write_atomic16,
 	&tcm_opcode_compare_write,
 	&tcm_opcode_read_capacity,
 	&tcm_opcode_read_capacity16,
-- 
2.34.1

[PATCH 7/7] scsi: target: Add atomic support to target_core_iblock

[Mike Christie]

This has target_core_iblock use the LIO helper function to translate its
block_device atomic settings to LIO settings. If we then get a write
that LIO has indicated is atomic via the SCF_ATOMIC flag, we use the
REQ_ATOMIC flag to tell the block layer to perform an atomic write.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 3a191bc35e04..afcd62f81d20 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -147,6 +147,8 @@ static int iblock_configure_device(struct se_device *dev)
 	if (bdev_nonrot(bd))
 		dev->dev_attrib.is_nonrot = 1;
 
+	target_configure_write_atomic_from_bdev(&dev->dev_attrib, bd);
+
 	bi = bdev_get_integrity(bd);
 	if (!bi)
 		return 0;
@@ -762,6 +764,9 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist *sgl, u32 sgl_nents,
 			else if (!bdev_write_cache(ib_dev->ibd_bd))
 				opf |= REQ_FUA;
 		}
+
+		if (cmd->se_cmd_flags & SCF_ATOMIC)
+			opf |= REQ_ATOMIC;
 	} else {
 		opf = REQ_OP_READ;
 		miter_dir = SG_MITER_FROM_SG;
-- 
2.34.1

Re: [PATCH 4/7] scsi: target: Add WRITE_ATOMIC_16 handler

[kernel test robot]

Hi Mike,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on jejb-scsi/for-next linus/master v6.12-rc2 next-20241008]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mike-Christie/scsi-target-Rename-target_configure_unmap_from_queue/20241008-101218
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20241008020437.78788-5-michael.christie%40oracle.com
patch subject: [PATCH 4/7] scsi: target: Add WRITE_ATOMIC_16 handler
config: x86_64-randconfig-123-20241009 (https://download.01.org/0day-ci/archive/20241009/202410091340.dQ10fC3u-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241009/202410091340.dQ10fC3u-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202410091340.dQ10fC3u-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/target/target_core_sbc.c:775:24: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:775:24: sparse:     expected int
   drivers/target/target_core_sbc.c:775:24: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:783:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:783:32: sparse:     expected int
   drivers/target/target_core_sbc.c:783:32: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:786:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:786:32: sparse:     expected int
   drivers/target/target_core_sbc.c:786:32: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:789:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:789:32: sparse:     expected int
   drivers/target/target_core_sbc.c:789:32: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:794:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:794:32: sparse:     expected int
   drivers/target/target_core_sbc.c:794:32: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:797:32: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:797:32: sparse:     expected int
   drivers/target/target_core_sbc.c:797:32: sparse:     got restricted sense_reason_t
   drivers/target/target_core_sbc.c:802:24: sparse: sparse: incorrect type in return expression (different base types) @@     expected int @@     got restricted sense_reason_t @@
   drivers/target/target_core_sbc.c:802:24: sparse:     expected int
   drivers/target/target_core_sbc.c:802:24: sparse:     got restricted sense_reason_t
>> drivers/target/target_core_sbc.c:919:29: sparse: sparse: incorrect type in assignment (different base types) @@     expected restricted sense_reason_t [assigned] [usertype] ret @@     got int @@
   drivers/target/target_core_sbc.c:919:29: sparse:     expected restricted sense_reason_t [assigned] [usertype] ret
   drivers/target/target_core_sbc.c:919:29: sparse:     got int

vim +775 drivers/target/target_core_sbc.c

   766	
   767	static int
   768	sbc_check_atomic(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
   769	{
   770		struct se_dev_attrib *attrib = &dev->dev_attrib;
   771		u16 boundary, transfer_len;
   772		u32 lba;
   773	
   774		if (!attrib->atomic_supported)
 > 775			return TCM_UNSUPPORTED_SCSI_OPCODE;
   776	
   777		lba = transport_lba_32(cdb);
   778		boundary = get_unaligned_be16(&cdb[10]);
   779		transfer_len = get_unaligned_be16(&cdb[12]);
   780	
   781		if (!boundary) {
   782			if (transfer_len > attrib->atomic_max_len)
   783				return TCM_INVALID_CDB_FIELD;
   784		} else {
   785			if (transfer_len > attrib->atomic_max_with_boundary)
   786				return TCM_INVALID_CDB_FIELD;
   787	
   788			if (boundary > attrib->atomic_max_boundary)
   789				return TCM_INVALID_CDB_FIELD;
   790		}
   791	
   792		if (attrib->atomic_granularity) {
   793			if (transfer_len % attrib->atomic_granularity)
   794				return TCM_INVALID_CDB_FIELD;
   795	
   796			if (boundary && boundary % attrib->atomic_granularity)
   797				return TCM_INVALID_CDB_FIELD;
   798		}
   799	
   800		if (dev->dev_attrib.atomic_alignment &&
   801		    lba % dev->dev_attrib.atomic_alignment)
   802			return TCM_INVALID_CDB_FIELD;
   803	
   804		return 0;
   805	}
   806	
   807	sense_reason_t
   808	sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
   809	{
   810		struct se_device *dev = cmd->se_dev;
   811		unsigned char *cdb = cmd->t_task_cdb;
   812		unsigned int size;
   813		u32 sectors = 0;
   814		sense_reason_t ret;
   815	
   816		cmd->protocol_data = ops;
   817	
   818		switch (cdb[0]) {
   819		case READ_6:
   820			sectors = transport_get_sectors_6(cdb);
   821			cmd->t_task_lba = transport_lba_21(cdb);
   822			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   823			cmd->execute_cmd = sbc_execute_rw;
   824			break;
   825		case READ_10:
   826			sectors = transport_get_sectors_10(cdb);
   827			cmd->t_task_lba = transport_lba_32(cdb);
   828	
   829			if (sbc_check_dpofua(dev, cmd, cdb))
   830				return TCM_INVALID_CDB_FIELD;
   831	
   832			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, false);
   833			if (ret)
   834				return ret;
   835	
   836			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   837			cmd->execute_cmd = sbc_execute_rw;
   838			break;
   839		case READ_12:
   840			sectors = transport_get_sectors_12(cdb);
   841			cmd->t_task_lba = transport_lba_32(cdb);
   842	
   843			if (sbc_check_dpofua(dev, cmd, cdb))
   844				return TCM_INVALID_CDB_FIELD;
   845	
   846			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, false);
   847			if (ret)
   848				return ret;
   849	
   850			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   851			cmd->execute_cmd = sbc_execute_rw;
   852			break;
   853		case READ_16:
   854			sectors = transport_get_sectors_16(cdb);
   855			cmd->t_task_lba = transport_lba_64(cdb);
   856	
   857			if (sbc_check_dpofua(dev, cmd, cdb))
   858				return TCM_INVALID_CDB_FIELD;
   859	
   860			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, false);
   861			if (ret)
   862				return ret;
   863	
   864			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   865			cmd->execute_cmd = sbc_execute_rw;
   866			break;
   867		case WRITE_6:
   868			sectors = transport_get_sectors_6(cdb);
   869			cmd->t_task_lba = transport_lba_21(cdb);
   870			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   871			cmd->execute_cmd = sbc_execute_rw;
   872			break;
   873		case WRITE_10:
   874		case WRITE_VERIFY:
   875			sectors = transport_get_sectors_10(cdb);
   876			cmd->t_task_lba = transport_lba_32(cdb);
   877	
   878			if (sbc_check_dpofua(dev, cmd, cdb))
   879				return TCM_INVALID_CDB_FIELD;
   880	
   881			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, true);
   882			if (ret)
   883				return ret;
   884	
   885			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   886			cmd->execute_cmd = sbc_execute_rw;
   887			break;
   888		case WRITE_12:
   889			sectors = transport_get_sectors_12(cdb);
   890			cmd->t_task_lba = transport_lba_32(cdb);
   891	
   892			if (sbc_check_dpofua(dev, cmd, cdb))
   893				return TCM_INVALID_CDB_FIELD;
   894	
   895			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, true);
   896			if (ret)
   897				return ret;
   898	
   899			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   900			cmd->execute_cmd = sbc_execute_rw;
   901			break;
   902		case WRITE_16:
   903		case WRITE_VERIFY_16:
   904		case WRITE_ATOMIC_16:
   905			sectors = transport_get_sectors_16(cdb);
   906			cmd->t_task_lba = transport_lba_64(cdb);
   907	
   908			if (sbc_check_dpofua(dev, cmd, cdb))
   909				return TCM_INVALID_CDB_FIELD;
   910	
   911			ret = sbc_check_prot(dev, cmd, cdb[1] >> 5, sectors, true);
   912			if (ret)
   913				return ret;
   914	
   915			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
   916			if (cdb[0] == WRITE_ATOMIC_16) {
   917				cmd->se_cmd_flags |= SCF_ATOMIC;
   918	
 > 919				ret = sbc_check_atomic(dev, cmd, cdb);
   920				if (ret)
   921					return ret;
   922			}
   923			cmd->execute_cmd = sbc_execute_rw;
   924			break;
   925		case VARIABLE_LENGTH_CMD:
   926		{
   927			u16 service_action = get_unaligned_be16(&cdb[8]);
   928			switch (service_action) {
   929			case WRITE_SAME_32:
   930				sectors = transport_get_sectors_32(cdb);
   931				if (!sectors) {
   932					pr_err("WSNZ=1, WRITE_SAME w/sectors=0 not"
   933					       " supported\n");
   934					return TCM_INVALID_CDB_FIELD;
   935				}
   936	
   937				size = sbc_get_size(cmd, 1);
   938				cmd->t_task_lba = get_unaligned_be64(&cdb[12]);
   939	
   940				ret = sbc_setup_write_same(cmd, cdb[10], ops);
   941				if (ret)
   942					return ret;
   943				break;
   944			default:
   945				pr_err("VARIABLE_LENGTH_CMD service action"
   946					" 0x%04x not supported\n", service_action);
   947				return TCM_UNSUPPORTED_SCSI_OPCODE;
   948			}
   949			break;
   950		}
   951		case COMPARE_AND_WRITE:
   952			if (!dev->dev_attrib.emulate_caw) {
   953				pr_err_ratelimited("se_device %s/%s (vpd_unit_serial %s) reject COMPARE_AND_WRITE\n",
   954						   dev->se_hba->backend->ops->name,
   955						   config_item_name(&dev->dev_group.cg_item),
   956						   dev->t10_wwn.unit_serial);
   957				return TCM_UNSUPPORTED_SCSI_OPCODE;
   958			}
   959			sectors = cdb[13];
   960			/*
   961			 * Currently enforce COMPARE_AND_WRITE for a single sector
   962			 */
   963			if (sectors > 1) {
   964				pr_err("COMPARE_AND_WRITE contains NoLB: %u greater"
   965				       " than 1\n", sectors);
   966				return TCM_INVALID_CDB_FIELD;
   967			}
   968			if (sbc_check_dpofua(dev, cmd, cdb))
   969				return TCM_INVALID_CDB_FIELD;
   970	
   971			/*
   972			 * Double size because we have two buffers, note that
   973			 * zero is not an error..
   974			 */
   975			size = 2 * sbc_get_size(cmd, sectors);
   976			cmd->t_task_lba = get_unaligned_be64(&cdb[2]);
   977			cmd->t_task_nolb = sectors;
   978			cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB | SCF_COMPARE_AND_WRITE;
   979			cmd->execute_cmd = sbc_compare_and_write;
   980			cmd->transport_complete_callback = compare_and_write_callback;
   981			break;
   982		case READ_CAPACITY:
   983			size = READ_CAP_LEN;
   984			cmd->execute_cmd = sbc_emulate_readcapacity;
   985			break;
   986		case SERVICE_ACTION_IN_16:
   987			switch (cmd->t_task_cdb[1] & 0x1f) {
   988			case SAI_READ_CAPACITY_16:
   989				cmd->execute_cmd = sbc_emulate_readcapacity_16;
   990				break;
   991			case SAI_REPORT_REFERRALS:
   992				cmd->execute_cmd = target_emulate_report_referrals;
   993				break;
   994			default:
   995				pr_err("Unsupported SA: 0x%02x\n",
   996					cmd->t_task_cdb[1] & 0x1f);
   997				return TCM_INVALID_CDB_FIELD;
   998			}
   999			size = get_unaligned_be32(&cdb[10]);
  1000			break;
  1001		case SYNCHRONIZE_CACHE:
  1002		case SYNCHRONIZE_CACHE_16:
  1003			if (cdb[0] == SYNCHRONIZE_CACHE) {
  1004				sectors = transport_get_sectors_10(cdb);
  1005				cmd->t_task_lba = transport_lba_32(cdb);
  1006			} else {
  1007				sectors = transport_get_sectors_16(cdb);
  1008				cmd->t_task_lba = transport_lba_64(cdb);
  1009			}
  1010			if (ops->execute_sync_cache) {
  1011				cmd->execute_cmd = ops->execute_sync_cache;
  1012				goto check_lba;
  1013			}
  1014			size = 0;
  1015			cmd->execute_cmd = sbc_emulate_noop;
  1016			break;
  1017		case UNMAP:
  1018			if (!ops->execute_unmap)
  1019				return TCM_UNSUPPORTED_SCSI_OPCODE;
  1020	
  1021			if (!dev->dev_attrib.emulate_tpu) {
  1022				pr_err("Got UNMAP, but backend device has"
  1023				       " emulate_tpu disabled\n");
  1024				return TCM_UNSUPPORTED_SCSI_OPCODE;
  1025			}
  1026			size = get_unaligned_be16(&cdb[7]);
  1027			cmd->execute_cmd = sbc_execute_unmap;
  1028			break;
  1029		case WRITE_SAME_16:
  1030			sectors = transport_get_sectors_16(cdb);
  1031			if (!sectors) {
  1032				pr_err("WSNZ=1, WRITE_SAME w/sectors=0 not supported\n");
  1033				return TCM_INVALID_CDB_FIELD;
  1034			}
  1035	
  1036			size = sbc_get_size(cmd, 1);
  1037			cmd->t_task_lba = get_unaligned_be64(&cdb[2]);
  1038	
  1039			ret = sbc_setup_write_same(cmd, cdb[1], ops);
  1040			if (ret)
  1041				return ret;
  1042			break;
  1043		case WRITE_SAME:
  1044			sectors = transport_get_sectors_10(cdb);
  1045			if (!sectors) {
  1046				pr_err("WSNZ=1, WRITE_SAME w/sectors=0 not supported\n");
  1047				return TCM_INVALID_CDB_FIELD;
  1048			}
  1049	
  1050			size = sbc_get_size(cmd, 1);
  1051			cmd->t_task_lba = get_unaligned_be32(&cdb[2]);
  1052	
  1053			/*
  1054			 * Follow sbcr26 with WRITE_SAME (10) and check for the existence
  1055			 * of byte 1 bit 3 UNMAP instead of original reserved field
  1056			 */
  1057			ret = sbc_setup_write_same(cmd, cdb[1], ops);
  1058			if (ret)
  1059				return ret;
  1060			break;
  1061		case VERIFY:
  1062		case VERIFY_16:
  1063			size = 0;
  1064			if (cdb[0] == VERIFY) {
  1065				sectors = transport_get_sectors_10(cdb);
  1066				cmd->t_task_lba = transport_lba_32(cdb);
  1067			} else {
  1068				sectors = transport_get_sectors_16(cdb);
  1069				cmd->t_task_lba = transport_lba_64(cdb);
  1070			}
  1071			cmd->execute_cmd = sbc_emulate_noop;
  1072			goto check_lba;
  1073		case REZERO_UNIT:
  1074		case SEEK_6:
  1075		case SEEK_10:
  1076			/*
  1077			 * There are still clients out there which use these old SCSI-2
  1078			 * commands. This mainly happens when running VMs with legacy
  1079			 * guest systems, connected via SCSI command pass-through to
  1080			 * iSCSI targets. Make them happy and return status GOOD.
  1081			 */
  1082			size = 0;
  1083			cmd->execute_cmd = sbc_emulate_noop;
  1084			break;
  1085		case START_STOP:
  1086			size = 0;
  1087			cmd->execute_cmd = sbc_emulate_startstop;
  1088			break;
  1089		default:
  1090			ret = spc_parse_cdb(cmd, &size);
  1091			if (ret)
  1092				return ret;
  1093		}
  1094	
  1095		/* reject any command that we don't have a handler for */
  1096		if (!cmd->execute_cmd)
  1097			return TCM_UNSUPPORTED_SCSI_OPCODE;
  1098	
  1099		if (cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
  1100			unsigned long long end_lba;
  1101	check_lba:
  1102			end_lba = dev->transport->get_blocks(dev) + 1;
  1103			if (((cmd->t_task_lba + sectors) < cmd->t_task_lba) ||
  1104			    ((cmd->t_task_lba + sectors) > end_lba)) {
  1105				pr_err("cmd exceeds last lba %llu "
  1106					"(lba %llu, sectors %u)\n",
  1107					end_lba, cmd->t_task_lba, sectors);
  1108				return TCM_ADDRESS_OUT_OF_RANGE;
  1109			}
  1110	
  1111			if (!(cmd->se_cmd_flags & SCF_COMPARE_AND_WRITE))
  1112				size = sbc_get_size(cmd, sectors);
  1113		}
  1114	
  1115		return target_cmd_size_check(cmd, size);
  1116	}
  1117	EXPORT_SYMBOL(sbc_parse_cdb);
  1118	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[John Garry]

On 08/10/2024 03:03, Mike Christie wrote:
> This adds atomic fields to the se_device and exports them in configfs.
> 
> Initially only target_core_iblock will be supported and we will inherit
> all the settings from the block layer except the alignment which userspace
> will set.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/target/target_core_configfs.c | 42 +++++++++++++++++++++++++++
>   include/target/target_core_base.h     |  6 ++++
>   2 files changed, 48 insertions(+)
> 
> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
> index c40217f44b1b..f3c7da650f65 100644
> --- a/drivers/target/target_core_configfs.c
> +++ b/drivers/target/target_core_configfs.c
> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>   DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>   DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>   DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>   
>   #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)				\
>   static ssize_t _name##_store(struct config_item *item, const char *page,\
> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct config_item *item, const char *page,
>   	return count;
>   }
>   
> +static ssize_t atomic_alignment_store(struct config_item *item,
> +				      const char *page, size_t count)

What is special about alignment that we need to be able to configure this?

Won't that be derived from the block device queue_limits (like granularity)?

> +{
> +	struct se_dev_attrib *da = to_attrib(item);
> +	u32 val;
> +	int ret;
> +
> +	ret = kstrtou32(page, 0, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (da->da_dev->export_count) {
> +		pr_err("dev[%p]: Unable to change SE Device atomic_alignment while export_count is %d\n",
> +		       da->da_dev, da->da_dev->export_count);
> +		return -EINVAL;
> +	}
> +
> +	da->atomic_alignment = val;
> +
> +	pr_debug("dev[%p]: SE Device atomic_alignment changed to %u\n",
> +		 da->da_dev, val);
> +	return count;
> +}
> +
>   CONFIGFS_ATTR(, emulate_model_alias);
>   CONFIGFS_ATTR(, emulate_dpo);
>   CONFIGFS_ATTR(, emulate_fua_write);
> @@ -1302,6 +1332,12 @@ CONFIGFS_ATTR(, max_write_same_len);
>   CONFIGFS_ATTR(, alua_support);
>   CONFIGFS_ATTR(, pgr_support);
>   CONFIGFS_ATTR(, submit_type);
> +CONFIGFS_ATTR(, atomic_alignment);
> +CONFIGFS_ATTR_RO(, atomic_supported);

isn't a non-zero atomic_max_len the same thing as this?

> +CONFIGFS_ATTR_RO(, atomic_max_len);
> +CONFIGFS_ATTR_RO(, atomic_granularity);
> +CONFIGFS_ATTR_RO(, atomic_max_with_boundary);
> +CONFIGFS_ATTR_RO(, atomic_max_boundary);
>   
>   /*
>    * dev_attrib attributes for devices using the target core SBC/SPC
> @@ -1345,6 +1381,12 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
>   	&attr_pgr_support,
>   	&attr_emulate_rsoc,
>   	&attr_submit_type,
> +	&attr_atomic_supported,
> +	&attr_atomic_alignment,
> +	&attr_atomic_max_len,
> +	&attr_atomic_granularity,
> +	&attr_atomic_max_with_boundary,
> +	&attr_atomic_max_boundary,
>   	NULL,
>   };
>   EXPORT_SYMBOL(sbc_attrib_attrs);
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 97099a5e3f6c..6c87bd018983 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -715,6 +715,7 @@ struct se_dev_attrib {
>   	bool		is_nonrot;
>   	bool		emulate_rest_reord;
>   	bool		unmap_zeroes_data;
> +	bool		atomic_supported;
>   	u32		hw_block_size;
>   	u32		block_size;
>   	u32		hw_max_sectors;
> @@ -726,6 +727,11 @@ struct se_dev_attrib {
>   	u32		unmap_granularity;
>   	u32		unmap_granularity_alignment;
>   	u32		max_write_same_len;
> +	u32		atomic_max_len;
> +	u32		atomic_alignment;
> +	u32		atomic_granularity;
> +	u32		atomic_max_with_boundary;
> +	u32		atomic_max_boundary;

these will always be zero, right? I don't see much point in even storing 
them.

>   	u8		submit_type;
>   	struct se_device *da_dev;
>   	struct config_group da_group;

Re: [PATCH 3/7] scsi: target: Add helper to setup atomic values from block_device

[John Garry]

On 08/10/2024 03:03, Mike Christie wrote:
> This adds a helper function that sets up the atomic value based on a
> block_device similar to what we do for unmap.

looks ok, apart from earlier query on alignment

> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/target/target_core_device.c  | 18 ++++++++++++++++++
>   include/target/target_core_backend.h |  2 ++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index b709a64c0ff3..4741de3d1061 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -793,6 +793,24 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
>   	return dev;
>   }
>   
> +void target_configure_write_atomic_from_bdev(struct se_dev_attrib *attrib,
> +					     struct block_device *bdev)
> +{
> +	struct request_queue *q = bdev_get_queue(bdev);
> +	int block_size = bdev_logical_block_size(bdev);
> +
> +	if (!bdev_can_atomic_write(bdev))
> +		return;
> +
> +	attrib->atomic_supported = true;
> +	attrib->atomic_max_len = queue_atomic_write_max_bytes(q) / block_size;
> +	attrib->atomic_granularity = queue_atomic_write_unit_min_bytes(q) /
> +								block_size;
> +	attrib->atomic_max_with_boundary = 0;
> +	attrib->atomic_max_boundary = 0;
> +}
> +EXPORT_SYMBOL_GPL(target_configure_write_atomic_from_bdev);
> +
>   /*
>    * Check if the underlying struct block_device supports discard and if yes
>    * configure the UNMAP parameters.
> diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
> index d394306f8f49..e32de80854b6 100644
> --- a/include/target/target_core_backend.h
> +++ b/include/target/target_core_backend.h
> @@ -123,6 +123,8 @@ bool target_sense_desc_format(struct se_device *dev);
>   sector_t target_to_linux_sector(struct se_device *dev, sector_t lb);
>   bool target_configure_unmap_from_bdev(struct se_dev_attrib *attrib,
>   				      struct block_device *bdev);
> +void target_configure_write_atomic_from_bdev(struct se_dev_attrib *attrib,
> +					     struct block_device *bdev);
>   
>   static inline bool target_dev_configured(struct se_device *se_dev)
>   {

Re: [PATCH 4/7] scsi: target: Add WRITE_ATOMIC_16 handler

[John Garry]

On 08/10/2024 03:03, Mike Christie wrote:
> This adds the core LIO code to process the WRITE_ATOMIC_16 command.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/target/target_core_sbc.c  | 48 +++++++++++++++++++++++++++++++
>   include/target/target_core_base.h |  1 +
>   2 files changed, 49 insertions(+)
> 
> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
> index fe8beb7dbab1..0ae0d4ec0ac3 100644
> --- a/drivers/target/target_core_sbc.c
> +++ b/drivers/target/target_core_sbc.c
> @@ -764,6 +764,46 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
>   	return 0;
>   }
>   
> +static int
> +sbc_check_atomic(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
> +{
> +	struct se_dev_attrib *attrib = &dev->dev_attrib;
> +	u16 boundary, transfer_len;
> +	u32 lba;
> +
> +	if (!attrib->atomic_supported)
> +		return TCM_UNSUPPORTED_SCSI_OPCODE;
> +
> +	lba = transport_lba_32(cdb);

should this be 64, not 32?

> +	boundary = get_unaligned_be16(&cdb[10]);
> +	transfer_len = get_unaligned_be16(&cdb[12]);
> +
> +	if (!boundary) {
> +		if (transfer_len > attrib->atomic_max_len)
> +			return TCM_INVALID_CDB_FIELD;

from sbc4r22 4.29.1, the key is ILLEGAL_REQUEST and sense code is 
INVALID CDB FIELD, but is it possible to have the field pointer set to 
the TRANSFER LENGTH in the CDB?

> +	} else {

earlier we have no boundary support, so we should reject any IO with a 
boundary set, right? From a glance at sbc4r22 4.29.1, that's my take

> +		if (transfer_len > attrib->atomic_max_with_boundary)
> +			return TCM_INVALID_CDB_FIELD;
> +
> +		if (boundary > attrib->atomic_max_boundary)
> +			return TCM_INVALID_CDB_FIELD;
> +	}
> +
> +	if (attrib->atomic_granularity) {
> +		if (transfer_len % attrib->atomic_granularity)
> +			return TCM_INVALID_CDB_FIELD;
> +
> +		if (boundary && boundary % attrib->atomic_granularity)
> +			return TCM_INVALID_CDB_FIELD;
> +	}
> +
> +	if (dev->dev_attrib.atomic_alignment &&
> +	    lba % dev->dev_attrib.atomic_alignment)
> +		return TCM_INVALID_CDB_FIELD;
> +
> +	return 0;
> +}
> +
>   sense_reason_t
>   sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
>   {
> @@ -861,6 +901,7 @@ sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
>   		break;
>   	case WRITE_16:
>   	case WRITE_VERIFY_16:
> +	case WRITE_ATOMIC_16:
>   		sectors = transport_get_sectors_16(cdb);
>   		cmd->t_task_lba = transport_lba_64(cdb);
>   
> @@ -872,6 +913,13 @@ sbc_parse_cdb(struct se_cmd *cmd, struct exec_cmd_ops *ops)
>   			return ret;
>   
>   		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
> +		if (cdb[0] == WRITE_ATOMIC_16) {
> +			cmd->se_cmd_flags |= SCF_ATOMIC;
> +
> +			ret = sbc_check_atomic(dev, cmd, cdb);
> +			if (ret)
> +				return ret;
> +		}
>   		cmd->execute_cmd = sbc_execute_rw;
>   		break;
>   	case VARIABLE_LENGTH_CMD:
> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
> index 6c87bd018983..7b8ed21bea03 100644
> --- a/include/target/target_core_base.h
> +++ b/include/target/target_core_base.h
> @@ -157,6 +157,7 @@ enum se_cmd_flags_table {
>   	SCF_USE_CPUID				= (1 << 16),
>   	SCF_TASK_ATTR_SET			= (1 << 17),
>   	SCF_TREAT_READ_AS_NORMAL		= (1 << 18),
> +	SCF_ATOMIC				= (1 << 19),
>   };
>   
>   /*

Re: [PATCH 5/7] scsi: target: Report atomic values in INQUIRY

[John Garry]

> -	 */
> +	put_unaligned_be16(12, &buf[2]);
> +
>   	if (!have_tp)
> -		goto max_write_same;
> +		goto try_atomic;
>   
>   	/*
>   	 * Set MAXIMUM UNMAP LBA COUNT
> @@ -595,9 +593,29 @@ spc_emulate_evpd_b0(struct se_cmd *cmd, unsigned char *buf)
>   	/*
>   	 * MAXIMUM WRITE SAME LENGTH
>   	 */
> -max_write_same:
>   	put_unaligned_be64(dev->dev_attrib.max_write_same_len, &buf[36]);
>   
> +	put_unaligned_be16(40, &buf[2]);
> +
> +try_atomic:
> +	/*
> +	 * ATOMIC
> +	 */
> +	if (!dev->dev_attrib.atomic_supported)
> +		goto done;
 > +> +	if (dev->dev_attrib.atomic_max_len < io_max_blocks)
> +		put_unaligned_be32(dev->dev_attrib.atomic_max_len, &buf[44]);
> +	else
> +		put_unaligned_be32(io_max_blocks, &buf[44]);
> +
> +	put_unaligned_be32(dev->dev_attrib.atomic_alignment, &buf[48]);
> +	put_unaligned_be32(dev->dev_attrib.atomic_granularity, &buf[52]);
> +	put_unaligned_be32(dev->dev_attrib.atomic_max_with_boundary, &buf[56]);
> +	put_unaligned_be32(dev->dev_attrib.atomic_max_boundary, &buf[60]);
> +
> +	put_unaligned_be16(60, &buf[2]);

this is the page length, right? if so, can we always report 60 and 
ensure that the atomics and other fields are 0 when they should be? Or 
does that violate the spec?

> +done:
>   	return 0;
>   }
>   

Re: [PATCH 7/7] scsi: target: Add atomic support to target_core_iblock

[John Garry]

On 08/10/2024 03:03, Mike Christie wrote:
> This has target_core_iblock use the LIO helper function to translate its
> block_device atomic settings to LIO settings. If we then get a write
> that LIO has indicated is atomic via the SCF_ATOMIC flag, we use the
> REQ_ATOMIC flag to tell the block layer to perform an atomic write.
> 
> Signed-off-by: Mike Christie<michael.christie@oracle.com>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH 1/7] scsi: target: Rename target_configure_unmap_from_queue

[John Garry]

On 08/10/2024 03:03, Mike Christie wrote:
> Rename target_configure_unmap_from_queue to
> target_configure_unmap_from_bdev since it now takes a bdev.
> 
> Signed-off-by: Mike Christie<michael.christie@oracle.com>

Reviewed-by: John Garry <john.g.garry@oracle.com>

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[Mike Christie]

On 5/14/25 5:26 AM, John Garry wrote:
> On 08/10/2024 03:03, Mike Christie wrote:
>> This adds atomic fields to the se_device and exports them in configfs.
>>
>> Initially only target_core_iblock will be supported and we will inherit
>> all the settings from the block layer except the alignment which userspace
>> will set.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>   drivers/target/target_core_configfs.c | 42 +++++++++++++++++++++++++++
>>   include/target/target_core_base.h     |  6 ++++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>> index c40217f44b1b..f3c7da650f65 100644
>> --- a/drivers/target/target_core_configfs.c
>> +++ b/drivers/target/target_core_configfs.c
>> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>>   DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>>   DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>>   DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>>     #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)                \
>>   static ssize_t _name##_store(struct config_item *item, const char *page,\
>> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct config_item *item, const char *page,
>>       return count;
>>   }
>>   +static ssize_t atomic_alignment_store(struct config_item *item,
>> +                      const char *page, size_t count)
> 
> What is special about alignment that we need to be able to configure this?
> 
> Won't that be derived from the block device queue_limits (like granularity)?

What if you are not using a physical block device like using LIO's ramdisk mode
to perform testing?

> 
>> +{
>> +    struct se_dev_attrib *da = to_attrib(item);
>> +    u32 val;
>> +    int ret;
>> +
>> +    ret = kstrtou32(page, 0, &val);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    if (da->da_dev->export_count) {
>> +        pr_err("dev[%p]: Unable to change SE Device atomic_alignment while export_count is %d\n",
>> +               da->da_dev, da->da_dev->export_count);
>> +        return -EINVAL;
>> +    }
>> +
>> +    da->atomic_alignment = val;
>> +
>> +    pr_debug("dev[%p]: SE Device atomic_alignment changed to %u\n",
>> +         da->da_dev, val);
>> +    return count;
>> +}
>> +
>>   CONFIGFS_ATTR(, emulate_model_alias);
>>   CONFIGFS_ATTR(, emulate_dpo);
>>   CONFIGFS_ATTR(, emulate_fua_write);
>> @@ -1302,6 +1332,12 @@ CONFIGFS_ATTR(, max_write_same_len);
>>   CONFIGFS_ATTR(, alua_support);
>>   CONFIGFS_ATTR(, pgr_support);
>>   CONFIGFS_ATTR(, submit_type);
>> +CONFIGFS_ATTR(, atomic_alignment);
>> +CONFIGFS_ATTR_RO(, atomic_supported);
> 
> isn't a non-zero atomic_max_len the same thing as this?


I separated it because like above I was thinking maybe users will want to
use this to do some testing. So I tried to make it more generic.



> 
>> +CONFIGFS_ATTR_RO(, atomic_max_len);
>> +CONFIGFS_ATTR_RO(, atomic_granularity);
>> +CONFIGFS_ATTR_RO(, atomic_max_with_boundary);
>> +CONFIGFS_ATTR_RO(, atomic_max_boundary);
>>     /*
>>    * dev_attrib attributes for devices using the target core SBC/SPC
>> @@ -1345,6 +1381,12 @@ struct configfs_attribute *sbc_attrib_attrs[] = {
>>       &attr_pgr_support,
>>       &attr_emulate_rsoc,
>>       &attr_submit_type,
>> +    &attr_atomic_supported,
>> +    &attr_atomic_alignment,
>> +    &attr_atomic_max_len,
>> +    &attr_atomic_granularity,
>> +    &attr_atomic_max_with_boundary,
>> +    &attr_atomic_max_boundary,
>>       NULL,
>>   };
>>   EXPORT_SYMBOL(sbc_attrib_attrs);
>> diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
>> index 97099a5e3f6c..6c87bd018983 100644
>> --- a/include/target/target_core_base.h
>> +++ b/include/target/target_core_base.h
>> @@ -715,6 +715,7 @@ struct se_dev_attrib {
>>       bool        is_nonrot;
>>       bool        emulate_rest_reord;
>>       bool        unmap_zeroes_data;
>> +    bool        atomic_supported;
>>       u32        hw_block_size;
>>       u32        block_size;
>>       u32        hw_max_sectors;
>> @@ -726,6 +727,11 @@ struct se_dev_attrib {
>>       u32        unmap_granularity;
>>       u32        unmap_granularity_alignment;
>>       u32        max_write_same_len;
>> +    u32        atomic_max_len;
>> +    u32        atomic_alignment;
>> +    u32        atomic_granularity;
>> +    u32        atomic_max_with_boundary;
>> +    u32        atomic_max_boundary;
> 
> these will always be zero, right? I don't see much point in even storing them.
> 
If you use target_core_iblock yes. If you use some other backend then
it will be what's set in configfs.

Re: [PATCH 5/7] scsi: target: Report atomic values in INQUIRY

[Mike Christie]

On 5/14/25 5:56 AM, John Garry wrote:
>>
>> +    put_unaligned_be16(60, &buf[2]);
> 
> this is the page length, right? if so, can we always report 60 and ensure that the atomics and other fields are 0 when they should be? Or does that violate the spec?

I think it was something on the initiator side does not like that
where it uses 0 for the value instead of a OS's default.

Re: [PATCH 4/7] scsi: target: Add WRITE_ATOMIC_16 handler

[Mike Christie]

On 5/14/25 5:47 AM, John Garry wrote:
> On 08/10/2024 03:03, Mike Christie wrote:
>> This adds the core LIO code to process the WRITE_ATOMIC_16 command.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>   drivers/target/target_core_sbc.c  | 48 +++++++++++++++++++++++++++++++
>>   include/target/target_core_base.h |  1 +
>>   2 files changed, 49 insertions(+)
>>
>> diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
>> index fe8beb7dbab1..0ae0d4ec0ac3 100644
>> --- a/drivers/target/target_core_sbc.c
>> +++ b/drivers/target/target_core_sbc.c
>> @@ -764,6 +764,46 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
>>       return 0;
>>   }
>>   +static int
>> +sbc_check_atomic(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
>> +{
>> +    struct se_dev_attrib *attrib = &dev->dev_attrib;
>> +    u16 boundary, transfer_len;
>> +    u32 lba;
>> +
>> +    if (!attrib->atomic_supported)
>> +        return TCM_UNSUPPORTED_SCSI_OPCODE;
>> +
>> +    lba = transport_lba_32(cdb);
> 
> should this be 64, not 32?
> 

Yeah will fix.

>> +    boundary = get_unaligned_be16(&cdb[10]);
>> +    transfer_len = get_unaligned_be16(&cdb[12]);
>> +
>> +    if (!boundary) {
>> +        if (transfer_len > attrib->atomic_max_len)
>> +            return TCM_INVALID_CDB_FIELD;
> 
> from sbc4r22 4.29.1, the key is ILLEGAL_REQUEST and sense code is INVALID CDB FIELD, but is it possible to have the field pointer set to the TRANSFER LENGTH in the CDB?

I can. Will do.

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[John Garry]

On 20/07/2025 23:15, Mike Christie wrote:
> On 5/14/25 5:26 AM, John Garry wrote:
>> On 08/10/2024 03:03, Mike Christie wrote:
>>> This adds atomic fields to the se_device and exports them in configfs.
>>>
>>> Initially only target_core_iblock will be supported and we will inherit
>>> all the settings from the block layer except the alignment which userspace
>>> will set.
>>>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> ---
>>>    drivers/target/target_core_configfs.c | 42 +++++++++++++++++++++++++++
>>>    include/target/target_core_base.h     |  6 ++++
>>>    2 files changed, 48 insertions(+)
>>>
>>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/target_core_configfs.c
>>> index c40217f44b1b..f3c7da650f65 100644
>>> --- a/drivers/target/target_core_configfs.c
>>> +++ b/drivers/target/target_core_configfs.c
>>> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>>>    DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>>>    DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>>>    DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>>>      #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)                \
>>>    static ssize_t _name##_store(struct config_item *item, const char *page,\
>>> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct config_item *item, const char *page,
>>>        return count;
>>>    }
>>>    +static ssize_t atomic_alignment_store(struct config_item *item,
>>> +                      const char *page, size_t count)
>>
>> What is special about alignment that we need to be able to configure this?
>>
>> Won't that be derived from the block device queue_limits (like granularity)?
> 
> What if you are not using a physical block device like using LIO's ramdisk mode
> to perform testing?

But would this non-physical device still need to support atomics? I 
would assume so.

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[michael.christie]

On 7/23/25 6:28 AM, John Garry wrote:
> On 20/07/2025 23:15, Mike Christie wrote:
>> On 5/14/25 5:26 AM, John Garry wrote:
>>> On 08/10/2024 03:03, Mike Christie wrote:
>>>> This adds atomic fields to the se_device and exports them in configfs.
>>>>
>>>> Initially only target_core_iblock will be supported and we will inherit
>>>> all the settings from the block layer except the alignment which
>>>> userspace
>>>> will set.
>>>>
>>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>>> ---
>>>>    drivers/target/target_core_configfs.c | 42 ++++++++++++++++++++++
>>>> +++++
>>>>    include/target/target_core_base.h     |  6 ++++
>>>>    2 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/
>>>> target_core_configfs.c
>>>> index c40217f44b1b..f3c7da650f65 100644
>>>> --- a/drivers/target/target_core_configfs.c
>>>> +++ b/drivers/target/target_core_configfs.c
>>>> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>>>>    DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>>>>    DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>>>>    DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>>>>      #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)                \
>>>>    static ssize_t _name##_store(struct config_item *item, const char
>>>> *page,\
>>>> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct
>>>> config_item *item, const char *page,
>>>>        return count;
>>>>    }
>>>>    +static ssize_t atomic_alignment_store(struct config_item *item,
>>>> +                      const char *page, size_t count)
>>>
>>> What is special about alignment that we need to be able to configure
>>> this?
>>>
>>> Won't that be derived from the block device queue_limits (like
>>> granularity)?
>>
>> What if you are not using a physical block device like using LIO's
>> ramdisk mode
>> to perform testing?
> 
> But would this non-physical device still need to support atomics? I
> would assume so.

It depends. For just a testing mode not necessarily. It can be relaxed
so you can test different scenarios like you want to test when atomics
are not supported correctly. For real support yes.

But just to be clear for either case, there won't always be a
request_queue/block_device. For example, rd and tcmu don't use
target_configure_write_atomic_from_bdev, so the atomic values can be
whatever they want to support.

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[michael.christie]

On 7/23/25 10:10 AM, michael.christie@oracle.com wrote:
> On 7/23/25 6:28 AM, John Garry wrote:
>> On 20/07/2025 23:15, Mike Christie wrote:
>>> On 5/14/25 5:26 AM, John Garry wrote:
>>>> On 08/10/2024 03:03, Mike Christie wrote:
>>>>> This adds atomic fields to the se_device and exports them in configfs.
>>>>>
>>>>> Initially only target_core_iblock will be supported and we will inherit
>>>>> all the settings from the block layer except the alignment which
>>>>> userspace
>>>>> will set.
>>>>>
>>>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>>>> ---
>>>>>    drivers/target/target_core_configfs.c | 42 ++++++++++++++++++++++
>>>>> +++++
>>>>>    include/target/target_core_base.h     |  6 ++++
>>>>>    2 files changed, 48 insertions(+)
>>>>>
>>>>> diff --git a/drivers/target/target_core_configfs.c b/drivers/target/
>>>>> target_core_configfs.c
>>>>> index c40217f44b1b..f3c7da650f65 100644
>>>>> --- a/drivers/target/target_core_configfs.c
>>>>> +++ b/drivers/target/target_core_configfs.c
>>>>> @@ -578,6 +578,12 @@ DEF_CONFIGFS_ATTRIB_SHOW(unmap_zeroes_data);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(max_write_same_len);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(emulate_rsoc);
>>>>>    DEF_CONFIGFS_ATTRIB_SHOW(submit_type);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_supported);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_alignment);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_len);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_granularity);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_with_boundary);
>>>>> +DEF_CONFIGFS_ATTRIB_SHOW(atomic_max_boundary);
>>>>>      #define DEF_CONFIGFS_ATTRIB_STORE_U32(_name)                \
>>>>>    static ssize_t _name##_store(struct config_item *item, const char
>>>>> *page,\
>>>>> @@ -1266,6 +1272,30 @@ static ssize_t submit_type_store(struct
>>>>> config_item *item, const char *page,
>>>>>        return count;
>>>>>    }
>>>>>    +static ssize_t atomic_alignment_store(struct config_item *item,
>>>>> +                      const char *page, size_t count)
>>>>
>>>> What is special about alignment that we need to be able to configure
>>>> this?
>>>>
>>>> Won't that be derived from the block device queue_limits (like
>>>> granularity)?
>>>
>>> What if you are not using a physical block device like using LIO's
>>> ramdisk mode
>>> to perform testing?
>>
>> But would this non-physical device still need to support atomics? I
>> would assume so.
> 
> It depends. For just a testing mode not necessarily. It can be relaxed
> so you can test different scenarios like you want to test when atomics
> are not supported correctly. For real support yes.
> 
> But just to be clear for either case, there won't always be a
> request_queue/block_device. For example, rd and tcmu don't use
> target_configure_write_atomic_from_bdev, so the atomic values can be
> whatever they want to support.

I think I misunderstood the last comment.

I'm trying to make sure I can support a broken device for testing so I
tried to make as flexible as possible.

With the code as is though I think your concern in the last comment is
that a user could set some valid settings, use rd/tcmu, and think they
were safe when they are not. If so, then I agree that can happen.

Or are you saying even for broken device simulation that we will never
need to set values like alignment above?

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[John Garry]

On 23/07/2025 16:48, michael.christie@oracle.com wrote:
>>>> What if you are not using a physical block device like using LIO's
>>>> ramdisk mode
>>>> to perform testing?
>>> But would this non-physical device still need to support atomics? I
>>> would assume so.
>> It depends. For just a testing mode not necessarily. It can be relaxed
>> so you can test different scenarios like you want to test when atomics
>> are not supported correctly. For real support yes.
>>
>> But just to be clear for either case, there won't always be a
>> request_queue/block_device. For example, rd and tcmu don't use
>> target_configure_write_atomic_from_bdev, so the atomic values can be
>> whatever they want to support.

If that is acceptable, then I am curious why only make alignment 
configurable (and not the other params)? From below, it seems that you 
have some special device to support.

> I think I misunderstood the last comment.
> 
> I'm trying to make sure I can support a broken device for testing so I
> tried to make as flexible as possible.
> 
> With the code as is though I think your concern in the last comment is
> that a user could set some valid settings, use rd/tcmu, and think they
> were safe when they are not. If so, then I agree that can happen.
> 
> Or are you saying even for broken device simulation that we will never
> need to set values like alignment above?

I suppose that we can set alignment for broken devices, yes.

I'll make a couple of points about scsi alignment as it is a bit 
special. Firstly the alignment should be compatible with the atomic 
write unit min. atomic write unit min comes from the granularity/pbs. So 
the granularity/pbs needs to be compatible with alignment.

A further point is that if a partition is not aligned on the atomic 
write unit min boundary, then atomic writes are disabled for that bdev 
partition.

Re: [PATCH 2/7] scsi: target: Add atomic se_device fields

[michael.christie]

On 7/23/25 11:21 AM, John Garry wrote:
> On 23/07/2025 16:48, michael.christie@oracle.com wrote:
>>>>> What if you are not using a physical block device like using LIO's
>>>>> ramdisk mode
>>>>> to perform testing?
>>>> But would this non-physical device still need to support atomics? I
>>>> would assume so.
>>> It depends. For just a testing mode not necessarily. It can be relaxed
>>> so you can test different scenarios like you want to test when atomics
>>> are not supported correctly. For real support yes.
>>>
>>> But just to be clear for either case, there won't always be a
>>> request_queue/block_device. For example, rd and tcmu don't use
>>> target_configure_write_atomic_from_bdev, so the atomic values can be
>>> whatever they want to support.
> 
> If that is acceptable, then I am curious why only make alignment
> configurable (and not the other params)? From below, it seems that you
> have some special device to support.
> 

Oh I completely misunderstood you :) I see what you were asking. I think
it was just a mistake.

>> I think I misunderstood the last comment.
>>
>> I'm trying to make sure I can support a broken device for testing so I
>> tried to make as flexible as possible.
>>
>> With the code as is though I think your concern in the last comment is
>> that a user could set some valid settings, use rd/tcmu, and think they
>> were safe when they are not. If so, then I agree that can happen.
>>
>> Or are you saying even for broken device simulation that we will never
>> need to set values like alignment above?
> 
> I suppose that we can set alignment for broken devices, yes.
> 
> I'll make a couple of points about scsi alignment as it is a bit
> special. Firstly the alignment should be compatible with the atomic
> write unit min. atomic write unit min comes from the granularity/pbs. So
> the granularity/pbs needs to be compatible with alignment.
> 
> A further point is that if a partition is not aligned on the atomic
> write unit min boundary, then atomic writes are disabled for that bdev
> partition.

I didn't completely understand these 2 paragraphs. Are you saying you
want me to enforce this type of thing somewhere. How does the info above
come into play if we are testing against other OSs for example?