Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
On Mon 30-11-20 15:51:50, Christoph Hellwig wrote:
> On Mon, Nov 30, 2020 at 10:44:21AM +0100, Jan Kara wrote:
> > 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.
>
> I explained that I think the GENHD_FL_UP is the more useful one here in
> reply to your last comment. If the size changes to or from 0 during
> runtime we probably do want an event.
Ah, right, sorry, I missed that. And I agree that it might make sense for
changes to / from zero during runtime to send notification. But it still
seems as a thin ice with potential to breakage to me.
> But I'll add your hunk for now and we can discuss this separately.
OK, thanks. With that hunk feel free to add:
Reviewed-by: Jan Kara
Honza
--
Jan Kara
SUSE Labs, CR
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
On Mon, Nov 30, 2020 at 10:44:21AM +0100, Jan Kara wrote:
> 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.
I explained that I think the GENHD_FL_UP is the more useful one here in
reply to your last comment. If the size changes to or from 0 during
runtime we probably do want an event.
But I'll add your hunk for now and we can discuss this separately.
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
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
> Reviewed-by: Greg Kroah-Hartman
> Acked-by: Coly Li [bcache]
> Acked-by: Chao Yu [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
SUSE Labs, CR
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
On 11/28/20 5:14 PM, 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 Reviewed-by: Greg Kroah-Hartman Acked-by: Coly Li [bcache] Acked-by: Chao Yu [f2fs] --- block/bio.c| 4 +- block/blk-core.c | 2 +- block/blk.h| 53 -- block/genhd.c | 55 +++--- block/partitions/core.c| 17 --- drivers/block/loop.c | 1 - drivers/block/nbd.c| 2 +- drivers/block/xen-blkback/common.h | 4 +- drivers/md/bcache/super.c | 2 +- drivers/s390/block/dasd_ioctl.c| 4 +- drivers/target/target_core_pscsi.c | 5 +- fs/block_dev.c | 73 +- fs/f2fs/super.c| 2 +- fs/pstore/blk.c| 2 +- include/linux/genhd.h | 29 +++- kernel/trace/blktrace.c| 2 +- 16 files changed, 61 insertions(+), 196 deletions(-) Reviewed-by: Hannes Reinecke Cheers, Hannes -- Dr. Hannes ReineckeKernel Storage Architect h...@suse.de +49 911 74053 688 SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
On 2020/11/24 21:27, 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. Additional the field in hd_struct did not use proper serializiation, possibly allowing for torn writes. By only using the block_device field this problem also gets fixed. Signed-off-by: Christoph Hellwig Reviewed-by: Greg Kroah-Hartman --- block/bio.c| 4 +- block/blk-core.c | 2 +- block/blk.h| 53 -- block/genhd.c | 55 +++--- block/partitions/core.c| 17 --- drivers/block/loop.c | 1 - drivers/block/nbd.c| 2 +- drivers/block/xen-blkback/common.h | 4 +- drivers/md/bcache/super.c | 2 +- drivers/s390/block/dasd_ioctl.c| 4 +- drivers/target/target_core_pscsi.c | 7 +-- fs/block_dev.c | 73 +- fs/f2fs/super.c| 2 +- For f2fs part, Acked-by: Chao Yu Thanks, fs/pstore/blk.c| 2 +- include/linux/genhd.h | 29 +++- kernel/trace/blktrace.c| 2 +- 16 files changed, 60 insertions(+), 199 deletions(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Re: [dm-devel] [PATCH 30/45] block: remove the nr_sects field in struct hd_struct
On 11/24/20 9:27 PM, 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. > > Additional the field in hd_struct did not use proper serializiation, > possibly allowing for torn writes. By only using the block_device field > this problem also gets fixed. > > Signed-off-by: Christoph Hellwig > Reviewed-by: Greg Kroah-Hartman For the bcache part, Acked-by: Coly Li Thanks. Coly Li > --- > block/bio.c| 4 +- > block/blk-core.c | 2 +- > block/blk.h| 53 -- > block/genhd.c | 55 +++--- > block/partitions/core.c| 17 --- > drivers/block/loop.c | 1 - > drivers/block/nbd.c| 2 +- > drivers/block/xen-blkback/common.h | 4 +- > drivers/md/bcache/super.c | 2 +- > drivers/s390/block/dasd_ioctl.c| 4 +- > drivers/target/target_core_pscsi.c | 7 +-- > fs/block_dev.c | 73 +- > fs/f2fs/super.c| 2 +- > fs/pstore/blk.c| 2 +- > include/linux/genhd.h | 29 +++- > kernel/trace/blktrace.c| 2 +- > 16 files changed, 60 insertions(+), 199 deletions(-) > [snipped] > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > index c55d3c58a7ef55..04fa40868fbe10 100644 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -1408,7 +1408,7 @@ static int cached_dev_init(struct cached_dev *dc, > unsigned int block_size) > q->limits.raid_partial_stripes_expensive; > > ret = bcache_device_init(&dc->disk, block_size, > - dc->bdev->bd_part->nr_sects - dc->sb.data_offset, > + bdev_nr_sectors(dc->bdev) - dc->sb.data_offset, >dc->bdev, &bcache_cached_ops); > if (ret) > return ret; [snipped] -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
