diff mbox series

[2/2] scsi: target: iblock: Report space allocation errors

Message ID 20230517141537.80936-3-k.shelekhin@yadro.com (mailing list archive)
State New, archived
Headers show
Series scsi: target: iblock: Report space allocation errors | expand

Commit Message

Konstantin Shelekhin May 17, 2023, 2:15 p.m. UTC 
When a thin provisioned block device lacks free LBA it ends bio requests
with BLK_STS_NOSPC. Currently iblock treats bio status as a boolean and
terminates failed requests with LOGICAL UNIT COMMUNICATION FAILURE if
the status is non-zero. Thus, initiators see space allocation errors as
I/O errors.

This commit modifies the iblock_req structure to store the status of the
first failed bio instead of the total number of failed bios. The status
is then used to set the specific sense reason.

For BLK_STS_NOSPC the sense reason is set to TCM_SPACE_ALLOCATION_FAILED
as per SBC-3 4.7.3.6.

On Linux initiators:

old:

  $ dd if=/dev/zero of=/dev/sda oflag=direct bs=4k count=1
  dd: error writing '/dev/sda': I/O error

new:

  $ dd if=/dev/zero of=/dev/sda oflag=direct bs=4k count=1
  dd: error writing '/dev/sda': No space left on device

Signed-off-by: Konstantin Shelekhin <k.shelekhin@yadro.com>
Reviewed-by: Dmitry Bogdanov <d.bogdanov@yadro.com>
---
 drivers/target/target_core_iblock.c | 28 +++++++++++++++++++++-------
 drivers/target/target_core_iblock.h |  2 +-
 2 files changed, 22 insertions(+), 8 deletions(-)

Comments

kernel test robot May 19, 2023, 6:05 p.m. UTC | #1 
Hi Konstantin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on linus/master v6.4-rc2 next-20230519]
[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/Konstantin-Shelekhin/scsi-target-core-Add-sense-reason-for-space-allocation-errors/20230517-221755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20230517141537.80936-3-k.shelekhin%40yadro.com
patch subject: [PATCH 2/2] scsi: target: iblock: Report space allocation errors
config: sparc-randconfig-s043-20230517
compiler: sparc-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/a1c912b7cd68155b131113eae148f7f79ef93676
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Konstantin-Shelekhin/scsi-target-core-Add-sense-reason-for-space-allocation-errors/20230517-221755
        git checkout a1c912b7cd68155b131113eae148f7f79ef93676
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc SHELL=/bin/bash drivers/target/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305200106.isYmBYs5-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/target/target_core_iblock.c:333:57: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected restricted blk_status_t [usertype] status @@     got int @@
   drivers/target/target_core_iblock.c:333:57: sparse:     expected restricted blk_status_t [usertype] status
   drivers/target/target_core_iblock.c:333:57: sparse:     got int
>> drivers/target/target_core_iblock.c:354:61: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected int new @@     got restricted blk_status_t [usertype] bi_status @@
   drivers/target/target_core_iblock.c:354:61: sparse:     expected int new
   drivers/target/target_core_iblock.c:354:61: sparse:     got restricted blk_status_t [usertype] bi_status

vim +333 drivers/target/target_core_iblock.c

   324	
   325	static void iblock_complete_cmd(struct se_cmd *cmd)
   326	{
   327		struct iblock_req *ibr = cmd->priv;
   328		sense_reason_t reason;
   329	
   330		if (!refcount_dec_and_test(&ibr->pending))
   331			return;
   332	
 > 333		reason = iblock_blk_status_to_reason(atomic_read(&ibr->status));
   334	
   335		if (reason == TCM_NO_SENSE)
   336			target_complete_cmd(cmd, SAM_STAT_GOOD);
   337		else
   338			target_complete_cmd_with_sense(cmd, SAM_STAT_CHECK_CONDITION, reason);
   339	
   340		kfree(ibr);
   341	}
   342	
   343	static void iblock_bio_done(struct bio *bio)
   344	{
   345		struct se_cmd *cmd = bio->bi_private;
   346		struct iblock_req *ibr = cmd->priv;
   347	
   348		if (bio->bi_status) {
   349			pr_err("bio error: %p,  err: %d\n", bio, bio->bi_status);
   350			/*
   351			 * Set the error status of the iblock request to the error
   352			 * status of the first failed bio.
   353			 */
 > 354			atomic_cmpxchg(&ibr->status, BLK_STS_OK, bio->bi_status);
   355			smp_mb__after_atomic();
   356		}
   357	
   358		bio_put(bio);
   359	
   360		iblock_complete_cmd(cmd);
   361	}
   362
Mike Christie May 20, 2023, 6:05 p.m. UTC | #2 
On 5/17/23 9:15 AM, Konstantin Shelekhin wrote:
> When a thin provisioned block device lacks free LBA it ends bio requests
> with BLK_STS_NOSPC. Currently iblock treats bio status as a boolean and
> terminates failed requests with LOGICAL UNIT COMMUNICATION FAILURE if
> the status is non-zero. Thus, initiators see space allocation errors as
> I/O errors.
> 
> This commit modifies the iblock_req structure to store the status of the
> first failed bio instead of the total number of failed bios. The status
> is then used to set the specific sense reason.
> 

You posted this patch before right? I think it didn't get picked up because
the kernel bot keeps flagging the mixing of the blk_status_t and atomic_t.
Just build with sparse C=1 and you should see it.
Konstantin Shelekhin May 21, 2023, 6:28 p.m. UTC | #3 
On Sat, May 20, 2023 at 01:05:01PM -0500, Mike Christie wrote:
> On 5/17/23 9:15 AM, Konstantin Shelekhin wrote:
> > When a thin provisioned block device lacks free LBA it ends bio requests
> > with BLK_STS_NOSPC. Currently iblock treats bio status as a boolean and
> > terminates failed requests with LOGICAL UNIT COMMUNICATION FAILURE if
> > the status is non-zero. Thus, initiators see space allocation errors as
> > I/O errors.
> >
> > This commit modifies the iblock_req structure to store the status of the
> > first failed bio instead of the total number of failed bios. The status
> > is then used to set the specific sense reason.
> >
> 
> You posted this patch before right? I think it didn't get picked up because
> the kernel bot keeps flagging the mixing of the blk_status_t and atomic_t.
> Just build with sparse C=1 and you should see it.

Dammit, I thought I fixed this the last time. Will fix and resend.
kernel test robot May 27, 2023, 7:50 p.m. UTC | #4 
Hi Konstantin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on linus/master v6.4-rc3]
[cannot apply to next-20230525]
[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/Konstantin-Shelekhin/scsi-target-core-Add-sense-reason-for-space-allocation-errors/20230517-221755
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
patch link:    https://lore.kernel.org/r/20230517141537.80936-3-k.shelekhin%40yadro.com
patch subject: [PATCH 2/2] scsi: target: iblock: Report space allocation errors
config: sparc-randconfig-s043-20230517 (https://download.01.org/0day-ci/archive/20230528/202305280314.CoHoeIlx-lkp@intel.com/config)
compiler: sparc-linux-gcc (GCC) 12.1.0
reproduce:
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/a1c912b7cd68155b131113eae148f7f79ef93676
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Konstantin-Shelekhin/scsi-target-core-Add-sense-reason-for-space-allocation-errors/20230517-221755
        git checkout a1c912b7cd68155b131113eae148f7f79ef93676
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 ~/bin/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=sparc SHELL=/bin/bash drivers/target/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202305280314.CoHoeIlx-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/target/target_core_iblock.c:333:57: sparse: sparse: incorrect type in argument 1 (different base types) @@     expected restricted blk_status_t [usertype] status @@     got int @@
   drivers/target/target_core_iblock.c:333:57: sparse:     expected restricted blk_status_t [usertype] status
   drivers/target/target_core_iblock.c:333:57: sparse:     got int
>> drivers/target/target_core_iblock.c:354:61: sparse: sparse: incorrect type in argument 3 (different base types) @@     expected int new @@     got restricted blk_status_t [usertype] bi_status @@
   drivers/target/target_core_iblock.c:354:61: sparse:     expected int new
   drivers/target/target_core_iblock.c:354:61: sparse:     got restricted blk_status_t [usertype] bi_status

vim +333 drivers/target/target_core_iblock.c

   324	
   325	static void iblock_complete_cmd(struct se_cmd *cmd)
   326	{
   327		struct iblock_req *ibr = cmd->priv;
   328		sense_reason_t reason;
   329	
   330		if (!refcount_dec_and_test(&ibr->pending))
   331			return;
   332	
 > 333		reason = iblock_blk_status_to_reason(atomic_read(&ibr->status));
   334	
   335		if (reason == TCM_NO_SENSE)
   336			target_complete_cmd(cmd, SAM_STAT_GOOD);
   337		else
   338			target_complete_cmd_with_sense(cmd, SAM_STAT_CHECK_CONDITION, reason);
   339	
   340		kfree(ibr);
   341	}
   342	
   343	static void iblock_bio_done(struct bio *bio)
   344	{
   345		struct se_cmd *cmd = bio->bi_private;
   346		struct iblock_req *ibr = cmd->priv;
   347	
   348		if (bio->bi_status) {
   349			pr_err("bio error: %p,  err: %d\n", bio, bio->bi_status);
   350			/*
   351			 * Set the error status of the iblock request to the error
   352			 * status of the first failed bio.
   353			 */
 > 354			atomic_cmpxchg(&ibr->status, BLK_STS_OK, bio->bi_status);
   355			smp_mb__after_atomic();
   356		}
   357	
   358		bio_put(bio);
   359	
   360		iblock_complete_cmd(cmd);
   361	}
   362
diff mbox series

Patch

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index cc838ffd1294..78831a3df511 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -310,20 +310,33 @@  static sector_t iblock_get_blocks(struct se_device *dev)
 	return blocks_long;
 }
 
+static sense_reason_t iblock_blk_status_to_reason(blk_status_t status)
+{
+	switch (status) {
+	case BLK_STS_OK:
+		return TCM_NO_SENSE;
+	case BLK_STS_NOSPC:
+		return TCM_SPACE_ALLOCATION_FAILED;
+	default:
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+	}
+}
+
 static void iblock_complete_cmd(struct se_cmd *cmd)
 {
 	struct iblock_req *ibr = cmd->priv;
-	u8 status;
+	sense_reason_t reason;
 
 	if (!refcount_dec_and_test(&ibr->pending))
 		return;
 
-	if (atomic_read(&ibr->ib_bio_err_cnt))
-		status = SAM_STAT_CHECK_CONDITION;
+	reason = iblock_blk_status_to_reason(atomic_read(&ibr->status));
+
+	if (reason == TCM_NO_SENSE)
+		target_complete_cmd(cmd, SAM_STAT_GOOD);
 	else
-		status = SAM_STAT_GOOD;
+		target_complete_cmd_with_sense(cmd, SAM_STAT_CHECK_CONDITION, reason);
 
-	target_complete_cmd(cmd, status);
 	kfree(ibr);
 }
 
@@ -335,9 +348,10 @@  static void iblock_bio_done(struct bio *bio)
 	if (bio->bi_status) {
 		pr_err("bio error: %p,  err: %d\n", bio, bio->bi_status);
 		/*
-		 * Bump the ib_bio_err_cnt and release bio.
+		 * Set the error status of the iblock request to the error
+		 * status of the first failed bio.
 		 */
-		atomic_inc(&ibr->ib_bio_err_cnt);
+		atomic_cmpxchg(&ibr->status, BLK_STS_OK, bio->bi_status);
 		smp_mb__after_atomic();
 	}
 
diff --git a/drivers/target/target_core_iblock.h b/drivers/target/target_core_iblock.h
index 8c55375d2f75..fda2e41b2e74 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -13,7 +13,7 @@ 
 
 struct iblock_req {
 	refcount_t pending;
-	atomic_t ib_bio_err_cnt;
+	atomic_t status;
 } ____cacheline_aligned;
 
 #define IBDF_HAS_UDEV_PATH		0x01