Re: [PATCH v2] scsi: sd: block: Fix regressions in read-only block device handling

[Christoph Hellwig <hch@infradead.org>]

On Tue, Feb 12, 2019 at 09:57:17PM -0500, Martin K. Petersen wrote:
> Some devices come online in write protected state and switch to
> read-write once they are ready to process I/O requests. These devices
> broke with commit 20bd1d026aac ("scsi: sd: Keep disk read-only when
> re-reading partition") because we had no way to distinguish between a
> user decision to set a block_device read-only and the actual hardware
> device being write-protected.
> 
> Because partitions are dropped and recreated on revalidate we are
> unable to persist any user-provided policy in hd_struct. Introduce a
> bitmap in struct gendisk to track the user configuration. This bitmap
> is updated when BLKROSET is called on a given disk or partition.
> 
> A helper function, get_user_ro(), is provided to determine whether the
> ioctl has forced read-only state for a given block device. This helper
> is used by set_disk_ro() and add_partition() to ensure that both
> existing and newly created partitions will get the correct state.
> 
>  - If BLKROSET sets a whole disk device read-only, all partitions will
>    now end up in a read-only state.
> 
>  - If BLKROSET sets a given partition read-only, that partition will
>    remain read-only post revalidate.
> 
>  - Otherwise both the whole disk device and any partitions will
>    reflect the write protect state of the underlying device.
> 
> Cc: Jeremy Cline <jeremy@jcline.org>
> Cc: Oleksii Kurochko <olkuroch@cisco.com>
> Cc: stable@vger.kernel.org # v4.16+
> Reported-by: Oleksii Kurochko <olkuroch@cisco.com>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=201221
> Fixes: 20bd1d026aac ("scsi: sd: Keep disk read-only when re-reading partition")
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> ---
> 
> v2:
> 	- Track user read-only state in a bitmap
> 
> 	- Work around the regression that caused us to drop user
> 	  preferences on revalidate
> ---
>  block/genhd.c             | 22 +++++++++++++++++-----
>  block/ioctl.c             |  4 ++++
>  block/partition-generic.c |  2 +-
>  drivers/scsi/sd.c         |  4 +---
>  include/linux/genhd.h     |  2 ++
>  5 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/block/genhd.c b/block/genhd.c
> index 1dd8fd6613b8..34667eb1d3cc 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -1544,19 +1544,31 @@ void set_device_ro(struct block_device *bdev, int flag)
>  
>  EXPORT_SYMBOL(set_device_ro);
>  
> +bool get_user_ro(struct gendisk *disk, unsigned int partno)
> +{
> +	/* Is the user read-only bit set for the whole disk device? */
> +	if (test_bit(0, disk->user_ro_bitmap))
> +		return true;
> +
> +	/* Is the user read-only bit set for this particular partition? */
> +	if (test_bit(partno, disk->user_ro_bitmap))
> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(get_user_ro);

No need to export this function.

> +	p->policy = get_user_ro(disk, partno) ?: get_disk_ro(disk);

Can we avoid the obsfucating non-standard (GNU extension) use of ?: here?
Just use a local variable and a good old if.