diff mbox

scsi: sd: Implement blacklist option for WRITE SAME w/ UNMAP

Message ID 20170928013512.1058-1-martin.petersen@oracle.com (mailing list archive)
State Accepted
Headers show

Commit Message

Martin K. Petersen Sept. 28, 2017, 1:35 a.m. UTC
SBC-4 states:

  "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
   maximum number of LBAs that may be unmapped by an UNMAP command"

  "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
   the maximum number of contiguous logical blocks that the device server
   allows to be unmapped or written in a single WRITE SAME command."

Despite the spec being clear on the topic, some devices incorrectly
expect WRITE SAME commands with the UNMAP bit set to be limited to the
value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.

Implement a blacklist option that can be used to accommodate devices
with this behavior.

Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
Reported-by: Ewan D. Milne <emilne@redhat.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/scsi/scsi_scan.c    |  3 +++
 drivers/scsi/sd.c           | 16 ++++++++++++----
 include/scsi/scsi_device.h  |  1 +
 include/scsi/scsi_devinfo.h |  1 +
 4 files changed, 17 insertions(+), 4 deletions(-)

Comments

Ewan Milne Sept. 28, 2017, 3:46 p.m. UTC | #1
On Wed, 2017-09-27 at 21:35 -0400, Martin K. Petersen wrote:
> SBC-4 states:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates the
>    maximum number of LBAs that may be unmapped by an UNMAP command"
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value indicates
>    the maximum number of contiguous logical blocks that the device server
>    allows to be unmapped or written in a single WRITE SAME command."
> 
> Despite the spec being clear on the topic, some devices incorrectly
> expect WRITE SAME commands with the UNMAP bit set to be limited to the
> value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.
> 
> Implement a blacklist option that can be used to accommodate devices
> with this behavior.
> 
> Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
> Reported-by: Ewan D. Milne <emilne@redhat.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_scan.c    |  3 +++
>  drivers/scsi/sd.c           | 16 ++++++++++++----
>  include/scsi/scsi_device.h  |  1 +
>  include/scsi/scsi_devinfo.h |  1 +
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e7818afeda2b..15590a063ad9 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -956,6 +956,9 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	if (*bflags & BLIST_NO_DIF)
>  		sdev->no_dif = 1;
>  
> +	if (*bflags & BLIST_UNMAP_LIMIT_WS)
> +		sdev->unmap_limit_for_ws = 1;
> +
>  	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>  
>  	if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b18ba3235900..347be7580181 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -715,13 +715,21 @@ static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
>  		break;
>  
>  	case SD_LBP_WS16:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS16_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS16_BLOCKS);
>  		break;
>  
>  	case SD_LBP_WS10:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS10_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS10_BLOCKS);
>  		break;
>  
>  	case SD_LBP_ZERO:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 82e93ee94708..67c5a9f223f7 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -192,6 +192,7 @@ struct scsi_device {
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
>  	unsigned broken_fua:1;		/* Don't set FUA bit */
>  	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
> +	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit for WRITE SAME */
>  
>  	atomic_t disk_events_disable_depth; /* disable depth for disk events */
>  
> diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
> index 9592570e092a..36b03013d629 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -29,5 +29,6 @@
>  #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
>  #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
>  #define BLIST_MAX_1024		0x40000000 /* maximum 1024 sector cdb length */
> +#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit for WRITE SAME */
>  
>  #endif

Thanks.

Reviewed-by: Ewan D. Milne <emilne@redhat.com>
Laurence Oberman Sept. 29, 2017, 10:02 a.m. UTC | #2
On Wed, 2017-09-27 at 21:35 -0400, Martin K. Petersen wrote:
> SBC-4 states:
> 
>   "A MAXIMUM UNMAP LBA COUNT field set to a non-zero value indicates
> the
>    maximum number of LBAs that may be unmapped by an UNMAP command"
> 
>   "A MAXIMUM WRITE SAME LENGTH field set to a non-zero value
> indicates
>    the maximum number of contiguous logical blocks that the device
> server
>    allows to be unmapped or written in a single WRITE SAME command."
> 
> Despite the spec being clear on the topic, some devices incorrectly
> expect WRITE SAME commands with the UNMAP bit set to be limited to
> the
> value reported in MAXIMUM UNMAP LBA COUNT in the Block Limits VPD.
> 
> Implement a blacklist option that can be used to accommodate devices
> with this behavior.
> 
> Reported-by: Bill Kuzeja <William.Kuzeja@stratus.com>
> Reported-by: Ewan D. Milne <emilne@redhat.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/scsi/scsi_scan.c    |  3 +++
>  drivers/scsi/sd.c           | 16 ++++++++++++----
>  include/scsi/scsi_device.h  |  1 +
>  include/scsi/scsi_devinfo.h |  1 +
>  4 files changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index e7818afeda2b..15590a063ad9 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -956,6 +956,9 @@ static int scsi_add_lun(struct scsi_device *sdev,
> unsigned char *inq_result,
>  	if (*bflags & BLIST_NO_DIF)
>  		sdev->no_dif = 1;
>  
> +	if (*bflags & BLIST_UNMAP_LIMIT_WS)
> +		sdev->unmap_limit_for_ws = 1;
> +
>  	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
>  
>  	if (*bflags & BLIST_TRY_VPD_PAGES)
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b18ba3235900..347be7580181 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -715,13 +715,21 @@ static void sd_config_discard(struct scsi_disk
> *sdkp, unsigned int mode)
>  		break;
>  
>  	case SD_LBP_WS16:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS16_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks,
> (u32)SD_MAX_WS16_BLOCKS);
>  		break;
>  
>  	case SD_LBP_WS10:
> -		max_blocks = min_not_zero(sdkp->max_ws_blocks,
> -					  (u32)SD_MAX_WS10_BLOCKS);
> +		if (sdkp->device->unmap_limit_for_ws)
> +			max_blocks = sdkp->max_unmap_blocks;
> +		else
> +			max_blocks = sdkp->max_ws_blocks;
> +
> +		max_blocks = min_not_zero(max_blocks,
> (u32)SD_MAX_WS10_BLOCKS);
>  		break;
>  
>  	case SD_LBP_ZERO:
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 82e93ee94708..67c5a9f223f7 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -192,6 +192,7 @@ struct scsi_device {
>  	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled
> */
>  	unsigned broken_fua:1;		/* Don't set FUA bit
> */
>  	unsigned lun_in_cdb:1;		/* Store LUN bits in
> CDB[1] */
> +	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit
> for WRITE SAME */
>  
>  	atomic_t disk_events_disable_depth; /* disable depth for
> disk events */
>  
> diff --git a/include/scsi/scsi_devinfo.h
> b/include/scsi/scsi_devinfo.h
> index 9592570e092a..36b03013d629 100644
> --- a/include/scsi/scsi_devinfo.h
> +++ b/include/scsi/scsi_devinfo.h
> @@ -29,5 +29,6 @@
>  #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD
> pages */
>  #define BLIST_NO_RSOC		0x20000000 /* don't try to
> issue RSOC */
>  #define BLIST_MAX_1024		0x40000000 /* maximum 1024
> sector cdb length */
> +#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit
> for WRITE SAME */
>  
>  #endif

Hello Martin
I am testing this but its not being picked up so I want to know if I
have the kernel command line wrong here.

scsi_dev_flags=LIO-ORG:thin2:0x80000000

Device is here
[   16.853083] scsi 4:0:0:50: Direct-Access     LIO-
ORG  thin2            4.0  PQ: 0 ANSI: 5

Have a couple of printk's in now to see if I see the flags and they
dont trigger

        case SD_LBP_WS16:
                if (sdkp->device->unmap_limit_for_ws) {
                        max_blocks = sdkp->max_unmap_blocks;
                        printk("RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS16\n");
                }
                else
                        max_blocks = sdkp->max_ws_blocks;

                max_blocks = min_not_zero(max_blocks,
(u32)SD_MAX_WS16_BLOCKS);
                break;

        case SD_LBP_WS10:
                if (sdkp->device->unmap_limit_for_ws) {
                        max_blocks = sdkp->max_unmap_blocks;
                        printk("RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS10\n");
                }
                else
                        max_blocks = sdkp->max_ws_blocks;

                max_blocks = min_not_zero(max_blocks,
(u32)SD_MAX_WS10_BLOCKS);
                break;

What am I doing wrong to pass the BLIST flags.

Thanks
Laurence
Martin K. Petersen Sept. 29, 2017, 1:21 p.m. UTC | #3
Laurence,

> I am testing this but its not being picked up so I want to know if I
> have the kernel command line wrong here.
>
> scsi_dev_flags=LIO-ORG:thin2:0x80000000
>
> What am I doing wrong to pass the BLIST flags.

This worked for me:

[root@kvm ~]# echo "Linux:scsi_debug:0x80000000" > /proc/scsi/device_info
[root@kvm ~]# grep Linux /proc/scsi/device_info 
'Linux   ' 'scsi_debug      ' 0x80000000
[root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10 unmap_max_desc=1 write_same_length=20 lbpws=1
[root@kvm ~]# lsblk -D
NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
sda         0      512B       5K         0

(With the caveat that I tweaked scsi_debug to report the UNMAP
parameters despite lbpu being 0).
Laurence Oberman Sept. 29, 2017, 2:01 p.m. UTC | #4
On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> Laurence,
> 
> > I am testing this but its not being picked up so I want to know if
> > I
> > have the kernel command line wrong here.
> > 
> > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > 
> > What am I doing wrong to pass the BLIST flags.
> 
> This worked for me:
> 
> [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> /proc/scsi/device_info
> [root@kvm ~]# grep Linux /proc/scsi/device_info 
> 'Linux   ' 'scsi_debug      ' 0x80000000
> [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> unmap_max_desc=1 write_same_length=20 lbpws=1
> [root@kvm ~]# lsblk -D
> NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> sda         0      512B       5K         0
> 
> (With the caveat that I tweaked scsi_debug to report the UNMAP
> parameters despite lbpu being 0).
> 

OK, Thanks, that is working now and I pick up the correct size now.
Its going to be very useful for these corner case array
inconsistencies.

Tested-by: Laurence Oberman <loberman@redhat.com>

Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-Access     LIO-
ORG  thin2            4.0  PQ: 0 ANSI: 5
Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
implicit and explicit TPGS
Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi generic
sg64 type 0
Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set by
kernel flag for case SD_LBP_WS16
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-byte 
logical blocks: (41.9 GB/39.1 GiB)
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect is
off
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
enabled, read cache: enabled, supports DPO and FUA
Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition timeout
set to 60 seconds
Laurence Oberman Oct. 17, 2017, 2:26 p.m. UTC | #5
On Fri, 2017-09-29 at 10:01 -0400, Laurence Oberman wrote:
> On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> > Laurence,
> > 
> > > I am testing this but its not being picked up so I want to know
> > > if
> > > I
> > > have the kernel command line wrong here.
> > > 
> > > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > > 
> > > What am I doing wrong to pass the BLIST flags.
> > 
> > This worked for me:
> > 
> > [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> > /proc/scsi/device_info
> > [root@kvm ~]# grep Linux /proc/scsi/device_info 
> > 'Linux   ' 'scsi_debug      ' 0x80000000
> > [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> > unmap_max_desc=1 write_same_length=20 lbpws=1
> > [root@kvm ~]# lsblk -D
> > NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> > sda         0      512B       5K         0
> > 
> > (With the caveat that I tweaked scsi_debug to report the UNMAP
> > parameters despite lbpu being 0).
> > 
> 
> OK, Thanks, that is working now and I pick up the correct size now.
> Its going to be very useful for these corner case array
> inconsistencies.
> 
> Tested-by: Laurence Oberman <loberman@redhat.com>
> 
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-
> Access     LIO-
> ORG  thin2            4.0  PQ: 0 ANSI: 5
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
> implicit and explicit TPGS
> Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
> naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi generic
> sg64 type 0
> Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set by
> kernel flag for case SD_LBP_WS16
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-
> byte 
> logical blocks: (41.9 GB/39.1 GiB)
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect
> is
> off
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
> enabled, read cache: enabled, supports DPO and FUA
> Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition
> timeout
> set to 60 seconds


Hi Martin

We have the code accepted for the patch above and its working fine with
echo after boot as you already know.

However I am still fighting with trying to pass this on the kernel
command line. We need to be able to do this as a dynamic method for
adding devices to the list so that the list is populated prior to the
device scan.

Using scsi_dev_flags=LIO-ORG:thin2:0x80000000 on the kernel line is
ignored and not filled in.
Its apparent to me that we have no longer have code to actually copy
the string from the kernel line after boot.

I ran some tests and added a couple of printk;s to see if we have any
capture and its NULL.

So when did this stop working, is what I am now chasing

[    1.524595] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=
[    1.524705] RHDEBUG: In scsi_init_devinfo error=0

We have this in  drivers/scsi/scsi_devinfo.c

module_param_string(dev_flags, scsi_dev_flags, sizeof(scsi_dev_flags),
0);
MODULE_PARM_DESC(dev_flags,

         "Given scsi_dev_flags=vendor:model:flags[,v:m:f] add
black/white"
         " list entries for vendor and model with an integer value of
flags"
         " to the scsi device info list");

and we have:

/**
 * scsi_init_devinfo - set up the dynamic device list.
 *
 * Description:
 *      Add command line entries from scsi_dev_flags, then add
 *      scsi_static_device_list entries to the scsi device info list.
 */
int __init scsi_init_devinfo(void)
{
#ifdef CONFIG_SCSI_PROC_FS
        struct proc_dir_entry *p;
#endif
        int error, i;

        printk("RHDEBUG:In scsi_init_devinfo
scsi_dev_flags=%s\n",scsi_dev_flags);

        error = scsi_dev_info_add_list(SCSI_DEVINFO_GLOBAL, NULL);
        printk("RHDEBUG: In scsi_init_devinfo error=%d\n",error);
        if (error) {
                printk("RHDEBUG: In scsi_init_devinfo, calling
scsi_dev_info_add_list returning with error=%d\n",error);
                return error;
        }

        error = scsi_dev_info_list_add_str(scsi_dev_flags);
        if (error) {
                printk("RHDEBUG: In scsi_init_devinfo, calling
scsi_info_list_add returning with error=%d\n",error);
                goto out;
        }

        for (i = 0; scsi_static_device_list[i].vendor; i++) {
                error = scsi_dev_info_list_add(1 /* compatibile */,
                                scsi_static_device_list[i].vendor,
                                scsi_static_device_list[i].model,
                                NULL,
                                scsi_static_device_list[i].flags);
                if (error)
                        goto out;
        }

#ifdef CONFIG_SCSI_PROC_FS
        p = proc_create("scsi/device_info", 0, NULL,
&scsi_devinfo_proc_fops);
        if (!p) {
                error = -ENOMEM;
                goto out;
        }
#endif /* CONFIG_SCSI_PROC_FS */

 out:
        if (error)
                scsi_exit_devinfo();
        return error;
}


But I fail to see where we actually copy the string off the kernel
line.

I intend to add code and test and submit a patch but first wanted to
know if its me simply missing something here.

Thanks
Laurence
Laurence Oberman Oct. 17, 2017, 2:43 p.m. UTC | #6
On Tue, 2017-10-17 at 10:26 -0400, Laurence Oberman wrote:
> On Fri, 2017-09-29 at 10:01 -0400, Laurence Oberman wrote:
> > On Fri, 2017-09-29 at 09:21 -0400, Martin K. Petersen wrote:
> > > Laurence,
> > > 
> > > > I am testing this but its not being picked up so I want to know
> > > > if
> > > > I
> > > > have the kernel command line wrong here.
> > > > 
> > > > scsi_dev_flags=LIO-ORG:thin2:0x80000000
> > > > 
> > > > What am I doing wrong to pass the BLIST flags.
> > > 
> > > This worked for me:
> > > 
> > > [root@kvm ~]# echo "Linux:scsi_debug:0x80000000" >
> > > /proc/scsi/device_info
> > > [root@kvm ~]# grep Linux /proc/scsi/device_info 
> > > 'Linux   ' 'scsi_debug      ' 0x80000000
> > > [root@kvm ~]# modprobe scsi_debug unmap_max_blocks=10
> > > unmap_max_desc=1 write_same_length=20 lbpws=1
> > > [root@kvm ~]# lsblk -D
> > > NAME DISC-ALN DISC-GRAN DISC-MAX DISC-ZERO
> > > sda         0      512B       5K         0
> > > 
> > > (With the caveat that I tweaked scsi_debug to report the UNMAP
> > > parameters despite lbpu being 0).
> > > 
> > 
> > OK, Thanks, that is working now and I pick up the correct size now.
> > Its going to be very useful for these corner case array
> > inconsistencies.
> > 
> > Tested-by: Laurence Oberman <loberman@redhat.com>
> > 
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: Direct-
> > Access     LIO-
> > ORG  thin2            4.0  PQ: 0 ANSI: 5
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: supports
> > implicit and explicit TPGS
> > Sep 29 09:56:11 localhost kernel: scsi 1:0:0:50: alua: device
> > naa.6001405f7aa27ca453f4381a00f22ea6 port group 0 rel port 2
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: Attached scsi
> > generic
> > sg64 type 0
> > Sep 29 09:56:11 localhost kernel: RHDEBUG: unmap_limit_for_ws set
> > by
> > kernel flag for case SD_LBP_WS16
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] 81920000 512-
> > byte 
> > logical blocks: (41.9 GB/39.1 GiB)
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write Protect
> > is
> > off
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: [sdbl] Write cache:
> > enabled, read cache: enabled, supports DPO and FUA
> > Sep 29 09:56:11 localhost kernel: sd 1:0:0:50: alua: transition
> > timeout
> > set to 60 seconds
> 
> 
> Hi Martin
> 
> We have the code accepted for the patch above and its working fine
> with
> echo after boot as you already know.
> 
> However I am still fighting with trying to pass this on the kernel
> command line. We need to be able to do this as a dynamic method for
> adding devices to the list so that the list is populated prior to the
> device scan.
> 
> Using scsi_dev_flags=LIO-ORG:thin2:0x80000000 on the kernel line is
> ignored and not filled in.
> Its apparent to me that we have no longer have code to actually copy
> the string from the kernel line after boot.
> 
> I ran some tests and added a couple of printk;s to see if we have any
> capture and its NULL.
> 
> So when did this stop working, is what I am now chasing
> 
> [    1.524595] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=
> [    1.524705] RHDEBUG: In scsi_init_devinfo error=0
> 
> We have this in  drivers/scsi/scsi_devinfo.c
> 
> module_param_string(dev_flags, scsi_dev_flags,
> sizeof(scsi_dev_flags),
> 0);
> MODULE_PARM_DESC(dev_flags,
> 
>          "Given scsi_dev_flags=vendor:model:flags[,v:m:f] add
> black/white"
>          " list entries for vendor and model with an integer value of
> flags"
>          " to the scsi device info list");
> 
> and we have:
> 
> /**
>  * scsi_init_devinfo - set up the dynamic device list.
>  *
>  * Description:
>  *      Add command line entries from scsi_dev_flags, then add
>  *      scsi_static_device_list entries to the scsi device info list.
>  */
> int __init scsi_init_devinfo(void)
> {
> #ifdef CONFIG_SCSI_PROC_FS
>         struct proc_dir_entry *p;
> #endif
>         int error, i;
> 
>         printk("RHDEBUG:In scsi_init_devinfo
> scsi_dev_flags=%s\n",scsi_dev_flags);
> 
>         error = scsi_dev_info_add_list(SCSI_DEVINFO_GLOBAL, NULL);
>         printk("RHDEBUG: In scsi_init_devinfo error=%d\n",error);
>         if (error) {
>                 printk("RHDEBUG: In scsi_init_devinfo, calling
> scsi_dev_info_add_list returning with error=%d\n",error);
>                 return error;
>         }
> 
>         error = scsi_dev_info_list_add_str(scsi_dev_flags);
>         if (error) {
>                 printk("RHDEBUG: In scsi_init_devinfo, calling
> scsi_info_list_add returning with error=%d\n",error);
>                 goto out;
>         }
> 
>         for (i = 0; scsi_static_device_list[i].vendor; i++) {
>                 error = scsi_dev_info_list_add(1 /* compatibile */,
>                                 scsi_static_device_list[i].vendor,
>                                 scsi_static_device_list[i].model,
>                                 NULL,
>                                 scsi_static_device_list[i].flags);
>                 if (error)
>                         goto out;
>         }
> 
> #ifdef CONFIG_SCSI_PROC_FS
>         p = proc_create("scsi/device_info", 0, NULL,
> &scsi_devinfo_proc_fops);
>         if (!p) {
>                 error = -ENOMEM;
>                 goto out;
>         }
> #endif /* CONFIG_SCSI_PROC_FS */
> 
>  out:
>         if (error)
>                 scsi_exit_devinfo();
>         return error;
> }
> 
> 
> But I fail to see where we actually copy the string off the kernel
> line.
> 
> I intend to add code and test and submit a patch but first wanted to
> know if its me simply missing something here.
> 
> Thanks
> Laurence

Answering my own post here.
As soon as I sent this Ewan emailed me explaining what I was doing
wrong.

Its working now by using 
scsi_mod.use_blk_mq=y scsi_mod.dev_flags=LIO-ORG:thin2:0x80000000

[root@segstorage1 ~]# dmesg | grep RHDEBUG
[    1.498639] RHDEBUG:In scsi_init_devinfo scsi_dev_flags=LIO-
ORG:thin2:0x80000000
[    1.499003] RHDEBUG: In scsi_init_devinfo error=0
[    9.031071] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.202887] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.246251] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.423062] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.632754] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[    9.781824] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   15.706504] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.254131] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.373697] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.443442] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.503806] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   16.582369] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.484123] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.552131] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.692909] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   21.975010] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.153413] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.685256] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.687920] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.692079] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.697259] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.721023] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.724256] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16
[   22.728566] RHDEBUG: unmap_limit_for_ws set by kernel flag for case
SD_LBP_WS16

Sorry for the noise
Thanks
Laurence
diff mbox

Patch

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index e7818afeda2b..15590a063ad9 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -956,6 +956,9 @@  static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
 	if (*bflags & BLIST_NO_DIF)
 		sdev->no_dif = 1;
 
+	if (*bflags & BLIST_UNMAP_LIMIT_WS)
+		sdev->unmap_limit_for_ws = 1;
+
 	sdev->eh_timeout = SCSI_DEFAULT_EH_TIMEOUT;
 
 	if (*bflags & BLIST_TRY_VPD_PAGES)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b18ba3235900..347be7580181 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -715,13 +715,21 @@  static void sd_config_discard(struct scsi_disk *sdkp, unsigned int mode)
 		break;
 
 	case SD_LBP_WS16:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
-					  (u32)SD_MAX_WS16_BLOCKS);
+		if (sdkp->device->unmap_limit_for_ws)
+			max_blocks = sdkp->max_unmap_blocks;
+		else
+			max_blocks = sdkp->max_ws_blocks;
+
+		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS16_BLOCKS);
 		break;
 
 	case SD_LBP_WS10:
-		max_blocks = min_not_zero(sdkp->max_ws_blocks,
-					  (u32)SD_MAX_WS10_BLOCKS);
+		if (sdkp->device->unmap_limit_for_ws)
+			max_blocks = sdkp->max_unmap_blocks;
+		else
+			max_blocks = sdkp->max_ws_blocks;
+
+		max_blocks = min_not_zero(max_blocks, (u32)SD_MAX_WS10_BLOCKS);
 		break;
 
 	case SD_LBP_ZERO:
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 82e93ee94708..67c5a9f223f7 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -192,6 +192,7 @@  struct scsi_device {
 	unsigned no_dif:1;	/* T10 PI (DIF) should be disabled */
 	unsigned broken_fua:1;		/* Don't set FUA bit */
 	unsigned lun_in_cdb:1;		/* Store LUN bits in CDB[1] */
+	unsigned unmap_limit_for_ws:1;	/* Use the UNMAP limit for WRITE SAME */
 
 	atomic_t disk_events_disable_depth; /* disable depth for disk events */
 
diff --git a/include/scsi/scsi_devinfo.h b/include/scsi/scsi_devinfo.h
index 9592570e092a..36b03013d629 100644
--- a/include/scsi/scsi_devinfo.h
+++ b/include/scsi/scsi_devinfo.h
@@ -29,5 +29,6 @@ 
 #define BLIST_TRY_VPD_PAGES	0x10000000 /* Attempt to read VPD pages */
 #define BLIST_NO_RSOC		0x20000000 /* don't try to issue RSOC */
 #define BLIST_MAX_1024		0x40000000 /* maximum 1024 sector cdb length */
+#define BLIST_UNMAP_LIMIT_WS	0x80000000 /* Use UNMAP limit for WRITE SAME */
 
 #endif