Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

On Thu, 03/30 11:30, Martin K. Petersen wrote:
> Fam Zheng  writes:
> 
> >>rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
> >>   BLK_DEF_MAX_SECTORS);
> >
> > Yes, it is better. Is it okay to make the change when you apply?
> 
> Sure. Applied to 4.11/scsi-fixes.

Cool. Thanks!

Fam

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

On Thu, 03/30 11:30, Martin K. Petersen wrote:
> Fam Zheng  writes:
> 
> >>rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
> >>   BLK_DEF_MAX_SECTORS);
> >
> > Yes, it is better. Is it okay to make the change when you apply?
> 
> Sure. Applied to 4.11/scsi-fixes.

Cool. Thanks!

Fam

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Martin K. Petersen]

Fam Zheng  writes:

>>  rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
>>   BLK_DEF_MAX_SECTORS);
>
> Yes, it is better. Is it okay to make the change when you apply?

Sure. Applied to 4.11/scsi-fixes.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Martin K. Petersen]

Fam Zheng  writes:

>>  rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
>>   BLK_DEF_MAX_SECTORS);
>
> Yes, it is better. Is it okay to make the change when you apply?

Sure. Applied to 4.11/scsi-fixes.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

On Wed, 03/29 22:37, Martin K. Petersen wrote:
> Fam Zheng  writes:
> 
> Fam,
> 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index fcfeddc..a5c7e67 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> > rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> > } else
> > rw_max = BLK_DEF_MAX_SECTORS;
> > +   rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
> >  
> > /* Combine with controller limits */
> > q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
> 
> Instead of updating rw_max twice, how about:
> 
> } else
>   rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
>   BLK_DEF_MAX_SECTORS);

Yes, it is better. Is it okay to make the change when you apply?

Fam

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

On Wed, 03/29 22:37, Martin K. Petersen wrote:
> Fam Zheng  writes:
> 
> Fam,
> 
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index fcfeddc..a5c7e67 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
> > rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
> > } else
> > rw_max = BLK_DEF_MAX_SECTORS;
> > +   rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
> >  
> > /* Combine with controller limits */
> > q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
> 
> Instead of updating rw_max twice, how about:
> 
> } else
>   rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
>   BLK_DEF_MAX_SECTORS);

Yes, it is better. Is it okay to make the change when you apply?

Fam

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Martin K. Petersen]

Fam Zheng  writes:

Fam,

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fcfeddc..a5c7e67 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
>   } else
>   rw_max = BLK_DEF_MAX_SECTORS;
> + rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
>  
>   /* Combine with controller limits */
>   q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));

Instead of updating rw_max twice, how about:

} else
rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
  BLK_DEF_MAX_SECTORS);

?

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Martin K. Petersen]

Fam Zheng  writes:

Fam,

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index fcfeddc..a5c7e67 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
>   rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
>   } else
>   rw_max = BLK_DEF_MAX_SECTORS;
> + rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
>  
>   /* Combine with controller limits */
>   q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));

Instead of updating rw_max twice, how about:

} else
rw_max = min_not_zero(logical_to_sectors(sdp, dev_max),
  BLK_DEF_MAX_SECTORS);

?

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[kbuild test robot]

Hi Fam,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.11-rc4 next-20170327]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Fam-Zheng/sd-Consider-max_xfer_blocks-if-opt_xfer_blocks-is-unusable/20170328-141853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-x016-201713 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers//scsi/sd.c:35:
   drivers//scsi/sd.c: In function 'sd_revalidate_disk':
   include/linux/kernel.h:755:16: warning: comparison of distinct pointer types 
lacks a cast
 (void) ( == );   \
   ^
   include/linux/kernel.h:758:2: note: in expansion of macro '__min'
 __min(typeof(x), typeof(y),   \
 ^
>> include/linux/kernel.h:783:39: note: in expansion of macro 'min'
 __x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
  ^~~
>> drivers//scsi/sd.c:2960:11: note: in expansion of macro 'min_not_zero'
 rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  ^~~~
--
   In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers/scsi/sd.c:35:
   drivers/scsi/sd.c: In function 'sd_revalidate_disk':
   include/linux/kernel.h:755:16: warning: comparison of distinct pointer types 
lacks a cast
 (void) ( == );   \
   ^
   include/linux/kernel.h:758:2: note: in expansion of macro '__min'
 __min(typeof(x), typeof(y),   \
 ^
>> include/linux/kernel.h:783:39: note: in expansion of macro 'min'
 __x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
  ^~~
   drivers/scsi/sd.c:2960:11: note: in expansion of macro 'min_not_zero'
 rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  ^~~~

vim +/min_not_zero +2960 drivers//scsi/sd.c

  2944  dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
  2945  q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);
  2946  
  2947  /*
  2948   * Use the device's preferred I/O size for reads and writes
  2949   * unless the reported value is unreasonably small, large, or
  2950   * garbage.
  2951   */
  2952  if (sdkp->opt_xfer_blocks &&
  2953  sdkp->opt_xfer_blocks <= dev_max &&
  2954  sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
  2955  logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
  2956  q->limits.io_opt = logical_to_bytes(sdp, 
sdkp->opt_xfer_blocks);
  2957  rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
  2958  } else
  2959  rw_max = BLK_DEF_MAX_SECTORS;
> 2960  rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  2961  
  2962  /* Combine with controller limits */
  2963  q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
  2964  
  2965  set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
  2966  sd_config_write_same(sdkp);
  2967  kfree(buffer);
  2968  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[kbuild test robot]

Hi Fam,

[auto build test WARNING on scsi/for-next]
[also build test WARNING on v4.11-rc4 next-20170327]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Fam-Zheng/sd-Consider-max_xfer_blocks-if-opt_xfer_blocks-is-unusable/20170328-141853
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
config: x86_64-randconfig-x016-201713 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers//scsi/sd.c:35:
   drivers//scsi/sd.c: In function 'sd_revalidate_disk':
   include/linux/kernel.h:755:16: warning: comparison of distinct pointer types 
lacks a cast
 (void) ( == );   \
   ^
   include/linux/kernel.h:758:2: note: in expansion of macro '__min'
 __min(typeof(x), typeof(y),   \
 ^
>> include/linux/kernel.h:783:39: note: in expansion of macro 'min'
 __x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
  ^~~
>> drivers//scsi/sd.c:2960:11: note: in expansion of macro 'min_not_zero'
 rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  ^~~~
--
   In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers/scsi/sd.c:35:
   drivers/scsi/sd.c: In function 'sd_revalidate_disk':
   include/linux/kernel.h:755:16: warning: comparison of distinct pointer types 
lacks a cast
 (void) ( == );   \
   ^
   include/linux/kernel.h:758:2: note: in expansion of macro '__min'
 __min(typeof(x), typeof(y),   \
 ^
>> include/linux/kernel.h:783:39: note: in expansion of macro 'min'
 __x == 0 ? __y : ((__y == 0) ? __x : min(__x, __y)); })
  ^~~
   drivers/scsi/sd.c:2960:11: note: in expansion of macro 'min_not_zero'
 rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  ^~~~

vim +/min_not_zero +2960 drivers//scsi/sd.c

  2944  dev_max = min_not_zero(dev_max, sdkp->max_xfer_blocks);
  2945  q->limits.max_dev_sectors = logical_to_sectors(sdp, dev_max);
  2946  
  2947  /*
  2948   * Use the device's preferred I/O size for reads and writes
  2949   * unless the reported value is unreasonably small, large, or
  2950   * garbage.
  2951   */
  2952  if (sdkp->opt_xfer_blocks &&
  2953  sdkp->opt_xfer_blocks <= dev_max &&
  2954  sdkp->opt_xfer_blocks <= SD_DEF_XFER_BLOCKS &&
  2955  logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) {
  2956  q->limits.io_opt = logical_to_bytes(sdp, 
sdkp->opt_xfer_blocks);
  2957  rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
  2958  } else
  2959  rw_max = BLK_DEF_MAX_SECTORS;
> 2960  rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
  2961  
  2962  /* Combine with controller limits */
  2963  q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
  2964  
  2965  set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity));
  2966  sd_config_write_same(sdkp);
  2967  kfree(buffer);
  2968  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

[PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

If device reports a small max_xfer_blocks and a zero opt_xfer_blocks, we
end up using BLK_DEF_MAX_SECTORS, which is wrong and r/w of that size
may get error.

Fixes: ca369d51b3e ("block/sd: Fix device-imposed transfer length limits")
Signed-off-by: Fam Zheng 

---

v2: Fix granularity mismatch. [Martin]
---
 drivers/scsi/sd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fcfeddc..a5c7e67 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
} else
rw_max = BLK_DEF_MAX_SECTORS;
+   rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
 
/* Combine with controller limits */
q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
-- 
2.9.3

[PATCH v2] sd: Consider max_xfer_blocks if opt_xfer_blocks is unusable

[Fam Zheng]

If device reports a small max_xfer_blocks and a zero opt_xfer_blocks, we
end up using BLK_DEF_MAX_SECTORS, which is wrong and r/w of that size
may get error.

Fixes: ca369d51b3e ("block/sd: Fix device-imposed transfer length limits")
Signed-off-by: Fam Zheng 

---

v2: Fix granularity mismatch. [Martin]
---
 drivers/scsi/sd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index fcfeddc..a5c7e67 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2957,6 +2957,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks);
} else
rw_max = BLK_DEF_MAX_SECTORS;
+   rw_max = min_not_zero(rw_max, logical_to_sectors(sdp, dev_max));
 
/* Combine with controller limits */
q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q));
-- 
2.9.3