[dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct

[Jan Kara]

On Sat 28-11-20 17:14:55, Christoph Hellwig wrote:
> Now that the hd_struct always has a block device attached to it, there is
> no need for having two size field that just get out of sync.
> 
> Additionally the field in hd_struct did not use proper serialization,
> possibly allowing for torn writes.  By only using the block_device field
> this problem also gets fixed.
> 
> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> Acked-by: Coly Li <colyli at suse.de>			[bcache]
> Acked-by: Chao Yu <yuchao0 at huawei.com>			[f2fs]

...

> @@ -47,18 +57,22 @@ static void disk_release_events(struct gendisk *disk);
>  bool set_capacity_and_notify(struct gendisk *disk, sector_t size)
>  {
>  	sector_t capacity = get_capacity(disk);
> +	char *envp[] = { "RESIZE=1", NULL };
>  
>  	set_capacity(disk, size);
> -	revalidate_disk_size(disk, true);
> -
> -	if (capacity != size && capacity != 0 && size != 0) {
> -		char *envp[] = { "RESIZE=1", NULL };
> -
> -		kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
> -		return true;
> -	}
>  
> -	return false;
> +	/*
> +	 * Only print a message and send a uevent if the gendisk is user visible
> +	 * and alive.  This avoids spamming the log and udev when setting the
> +	 * initial capacity during probing.
> +	 */
> +	if (size == capacity ||
> +	    (disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
> +		return false;
> +	pr_info("%s: detected capacity change from %lld to %lld\n",
> +		disk->disk_name, size, capacity);
> +	kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
> +	return true;
>  }
>  EXPORT_SYMBOL_GPL(set_capacity_and_notify);

I know I'm like a broken record about this but I still don't understand
here... I'd expect the new code to be:

	if (size == capacity ||
	    (disk->flags & (GENHD_FL_UP | GENHD_FL_HIDDEN)) != GENHD_FL_UP)
		return false;
	pr_info("%s: detected capacity change from %lld to %lld\n",
		disk->disk_name, size, capacity);
+	if (!capacity || !size)
+		return false;
	kobject_uevent_env(&disk_to_dev(disk)->kobj, KOBJ_CHANGE, envp);
	return true;

At least that would be equivalent to the original functionality of
set_capacity_and_notify(). And if you indeed intend to change when
"RESIZE=1" events are sent, then I'd expect an explanation in the changelog
why this cannot break userspace (I remember we've already broken some udev
rules in the past by generating unexpected events and we had to revert
those changes in the partition code so I'm more careful now). The rest of
the patch looks good to me.

								Honza
-- 
Jan Kara <jack at suse.com>
SUSE Labs, CR