[PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down

[PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down

[Fam Zheng]

If issued right after link down, "blockdev --rereadpt" will be stuck for a
while and then return normally. Although the underlying capacity and partition
table are not correctly updated. And it means that userspace can't detect the
error at all.

Fix this by propargating the error of "read capacity" command through the
stack, so that the ioctl could fail with -EIO.

Fam Zheng (3):
  block: Return error in rescan_partitions if revalidating disk failed
  sd: Return error in sd_revalidate_disk if read capacity failed
  sd: Return -EIO if read capacity failed

 block/partition-generic.c |  6 +++---
 drivers/scsi/sd.c         | 22 +++++++++++++---------
 2 files changed, 16 insertions(+), 12 deletions(-)

-- 
1.9.3

[PATCH 1/3] block: Return error in rescan_partitions if revalidating disk failed

[Fam Zheng]

If the disk can't read capacity, we should return an error.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/partition-generic.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/block/partition-generic.c b/block/partition-generic.c
index 0d9e5f9..1e60d7d 100644
--- a/block/partition-generic.c
+++ b/block/partition-generic.c
@@ -427,11 +427,11 @@ rescan:
 		return res;
 
 	if (disk->fops->revalidate_disk)
-		disk->fops->revalidate_disk(disk);
+		res = disk->fops->revalidate_disk(disk);
 	check_disk_size_change(disk, bdev);
 	bdev->bd_invalidated = 0;
-	if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
-		return 0;
+	if (res || !get_capacity(disk) || !(state = check_partition(disk, bdev)))
+		return res;
 	if (IS_ERR(state)) {
 		/*
 		 * I/O error reading the partition table.  If any
-- 
1.9.3

Re: [PATCH 1/3] block: Return error in rescan_partitions if revalidating disk failed

[James Bottomley]

On Tue, 2015-03-24 at 18:16 +0800, Fam Zheng wrote:
> If the disk can't read capacity, we should return an error.

I'm not sure I buy this: you need to justify why.

For instance removable media return an error in revalidate that causes
the medium not present flag to be set ... do we really want to propagate
the error in that case?

> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/partition-generic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/block/partition-generic.c b/block/partition-generic.c
> index 0d9e5f9..1e60d7d 100644
> --- a/block/partition-generic.c
> +++ b/block/partition-generic.c
> @@ -427,11 +427,11 @@ rescan:
>  		return res;
>  
>  	if (disk->fops->revalidate_disk)
> -		disk->fops->revalidate_disk(disk);
> +		res = disk->fops->revalidate_disk(disk);

You're assuming here that a negative error code is being returned, but
some (cciss) seem to be returning 1 on error.

You'll need to assure us that you've checked all the consumers before
making this change.

James

>  	check_disk_size_change(disk, bdev);
>  	bdev->bd_invalidated = 0;
> -	if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
> -		return 0;
> +	if (res || !get_capacity(disk) || !(state = check_partition(disk, bdev)))
> +		return res;
>  	if (IS_ERR(state)) {
>  		/*
>  		 * I/O error reading the partition table.  If any

Re: [PATCH 1/3] block: Return error in rescan_partitions if revalidating disk failed

[Fam Zheng]

On Tue, 04/21 19:58, James Bottomley wrote:
> On Tue, 2015-03-24 at 18:16 +0800, Fam Zheng wrote:
> > If the disk can't read capacity, we should return an error.
> 
> I'm not sure I buy this: you need to justify why.
> 
> For instance removable media return an error in revalidate that causes
> the medium not present flag to be set ... do we really want to propagate
> the error in that case?

I think we should. There is a command failed, and the user won't get a valid
disk size.

> 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/partition-generic.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/block/partition-generic.c b/block/partition-generic.c
> > index 0d9e5f9..1e60d7d 100644
> > --- a/block/partition-generic.c
> > +++ b/block/partition-generic.c
> > @@ -427,11 +427,11 @@ rescan:
> >  		return res;
> >  
> >  	if (disk->fops->revalidate_disk)
> > -		disk->fops->revalidate_disk(disk);
> > +		res = disk->fops->revalidate_disk(disk);
> 
> You're assuming here that a negative error code is being returned, but
> some (cciss) seem to be returning 1 on error.
> 
> You'll need to assure us that you've checked all the consumers before
> making this change.

I'll look at the return values.

Fam

> 
> James
> 
> >  	check_disk_size_change(disk, bdev);
> >  	bdev->bd_invalidated = 0;
> > -	if (!get_capacity(disk) || !(state = check_partition(disk, bdev)))
> > -		return 0;
> > +	if (res || !get_capacity(disk) || !(state = check_partition(disk, bdev)))
> > +		return res;
> >  	if (IS_ERR(state)) {
> >  		/*
> >  		 * I/O error reading the partition table.  If any
> 
> 
> 

[PATCH 2/3] sd: Return error in sd_revalidate_disk if read capacity failed

[Fam Zheng]

When the read capacity commands failed, we should return an error to
caller instead of silently continuing as normal.

Most importantly, this fixes the error code of BLKRRPART ioctl. Also if
the device is down, the following commands also likely to time
out, and we could waste more time than necessary.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 drivers/scsi/sd.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6b78476..7e5ca3b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -113,7 +113,7 @@ static int sd_init_command(struct scsi_cmnd *SCpnt);
 static void sd_uninit_command(struct scsi_cmnd *SCpnt);
 static int sd_done(struct scsi_cmnd *);
 static int sd_eh_action(struct scsi_cmnd *, int);
-static void sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
+static int sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer);
 static void scsi_disk_release(struct device *cdev);
 static void sd_print_sense_hdr(struct scsi_disk *, struct scsi_sense_hdr *);
 static void sd_print_result(const struct scsi_disk *, const char *, int);
@@ -2145,7 +2145,7 @@ static int sd_try_rc16_first(struct scsi_device *sdp)
 /*
  * read disk capacity
  */
-static void
+static int
 sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 {
 	int sector_size;
@@ -2157,17 +2157,17 @@ sd_read_capacity(struct scsi_disk *sdkp, unsigned char *buffer)
 		if (sector_size == -EOVERFLOW)
 			goto got_data;
 		if (sector_size == -ENODEV)
-			return;
+			return -ENODEV;
 		if (sector_size < 0)
 			sector_size = read_capacity_10(sdkp, sdp, buffer);
 		if (sector_size < 0)
-			return;
+			return sector_size;
 	} else {
 		sector_size = read_capacity_10(sdkp, sdp, buffer);
 		if (sector_size == -EOVERFLOW)
 			goto got_data;
 		if (sector_size < 0)
-			return;
+			return sector_size;
 		if ((sizeof(sdkp->capacity) > 4) &&
 		    (sdkp->capacity > 0xffffffffULL)) {
 			int old_sector_size = sector_size;
@@ -2274,6 +2274,7 @@ got_data:
 	blk_queue_physical_block_size(sdp->request_queue,
 				      sdkp->physical_block_size);
 	sdkp->device->sector_size = sector_size;
+	return 0;
 }
 
 /* called with buffer of length 512 */
@@ -2753,6 +2754,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	struct scsi_device *sdp = sdkp->device;
 	unsigned char *buffer;
 	unsigned int max_xfer;
+	int ret = 0;
 
 	SCSI_LOG_HLQUEUE(3, sd_printk(KERN_INFO, sdkp,
 				      "sd_revalidate_disk\n"));
@@ -2778,7 +2780,9 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	 * react badly if we do.
 	 */
 	if (sdkp->media_present) {
-		sd_read_capacity(sdkp, buffer);
+		ret = sd_read_capacity(sdkp, buffer);
+		if (ret)
+			goto out;
 
 		if (sd_try_extended_inquiry(sdp)) {
 			sd_read_block_provisioning(sdkp);
@@ -2811,7 +2815,7 @@ static int sd_revalidate_disk(struct gendisk *disk)
 	kfree(buffer);
 
  out:
-	return 0;
+	return ret;
 }
 
 /**
-- 
1.9.3

[PATCH 3/3] sd: Return -EIO if read capacity failed

[Fam Zheng]

This improves the error code if BLKRRPART ioctl failed due to connection
issue.

Before:

	$ blockdev --rereadpt /dev/sda
	BLKRRPART: Invalid argument

After:
	$ blockdev --rereadpt /dev/sda
	BLKRRPART: Input/output error

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 drivers/scsi/sd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 7e5ca3b..bb0e38d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2019,7 +2019,7 @@ static int read_capacity_16(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	if (the_result) {
 		sd_print_result(sdkp, "Read Capacity(16) failed", the_result);
 		read_capacity_error(sdkp, sdp, &sshdr, sense_valid, the_result);
-		return -EINVAL;
+		return -EIO;
 	}
 
 	sector_size = get_unaligned_be32(&buffer[8]);
@@ -2101,7 +2101,7 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	if (the_result) {
 		sd_print_result(sdkp, "Read Capacity(10) failed", the_result);
 		read_capacity_error(sdkp, sdp, &sshdr, sense_valid, the_result);
-		return -EINVAL;
+		return -EIO;
 	}
 
 	sector_size = get_unaligned_be32(&buffer[4]);
-- 
1.9.3

Re: [PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down

[Paolo Bonzini]

On 24/03/2015 11:16, Fam Zheng wrote:
> If issued right after link down, "blockdev --rereadpt" will be stuck for a
> while and then return normally. Although the underlying capacity and partition
> table are not correctly updated. And it means that userspace can't detect the
> error at all.
> 
> Fix this by propargating the error of "read capacity" command through the
> stack, so that the ioctl could fail with -EIO.
> 
> Fam Zheng (3):
>   block: Return error in rescan_partitions if revalidating disk failed
>   sd: Return error in sd_revalidate_disk if read capacity failed
>   sd: Return -EIO if read capacity failed
> 
>  block/partition-generic.c |  6 +++---
>  drivers/scsi/sd.c         | 22 +++++++++++++---------
>  2 files changed, 16 insertions(+), 12 deletions(-)
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Though patch 3 could be seen as a change in userspace ABI, so I'm less
sure about it.

Paolo

Re: [PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down

[Fam Zheng]

On Tue, 03/24 11:34, Paolo Bonzini wrote:
> 
> 
> On 24/03/2015 11:16, Fam Zheng wrote:
> > If issued right after link down, "blockdev --rereadpt" will be stuck for a
> > while and then return normally. Although the underlying capacity and partition
> > table are not correctly updated. And it means that userspace can't detect the
> > error at all.
> > 
> > Fix this by propargating the error of "read capacity" command through the
> > stack, so that the ioctl could fail with -EIO.
> > 
> > Fam Zheng (3):
> >   block: Return error in rescan_partitions if revalidating disk failed
> >   sd: Return error in sd_revalidate_disk if read capacity failed
> >   sd: Return -EIO if read capacity failed
> > 
> >  block/partition-generic.c |  6 +++---
> >  drivers/scsi/sd.c         | 22 +++++++++++++---------
> >  2 files changed, 16 insertions(+), 12 deletions(-)
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Though patch 3 could be seen as a change in userspace ABI, so I'm less
> sure about it.
> 

The changed -EINVAL's are not to userspace before this series, so I think it is
OK.

Thanks for reviewing.

Fam

Re: [PATCH 0/3] Fix return code for ioctl( BLKRRPART ) if device is down

[Fam Zheng]

On Tue, 03/24 11:34, Paolo Bonzini wrote:
> 
> 
> On 24/03/2015 11:16, Fam Zheng wrote:
> > If issued right after link down, "blockdev --rereadpt" will be stuck for a
> > while and then return normally. Although the underlying capacity and partition
> > table are not correctly updated. And it means that userspace can't detect the
> > error at all.
> > 
> > Fix this by propargating the error of "read capacity" command through the
> > stack, so that the ioctl could fail with -EIO.
> > 
> > Fam Zheng (3):
> >   block: Return error in rescan_partitions if revalidating disk failed
> >   sd: Return error in sd_revalidate_disk if read capacity failed
> >   sd: Return -EIO if read capacity failed
> > 
> >  block/partition-generic.c |  6 +++---
> >  drivers/scsi/sd.c         | 22 +++++++++++++---------
> >  2 files changed, 16 insertions(+), 12 deletions(-)
> > 
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Ping?

> 
> Though patch 3 could be seen as a change in userspace ABI, so I'm less
> sure about it.
> 
> Paolo