On Fri 20-11-20 10:08:20, Christoph Hellwig wrote:
> On Thu, Nov 19, 2020 at 01:05:25PM +0100, Jan Kara wrote:
> > > @@ -613,7 +613,7 @@ void guard_bio_eod(struct bio *bio)
> > > rcu_read_lock();
> > > part = __disk_get_part(bio->bi_disk, bio->bi_partno);
> > > if (part)
> > > - maxsector = part_nr_sects_read(part);
> > > + maxsector = bdev_nr_sectors(part->bdev);
> > > else
> > > maxsector = get_capacity(bio->bi_disk);
> >
> > I have to say that after these changes I find it a bit confusing that we
> > have get/set_capacity() and bdev_nr_sectors() / bdev_set_nr_sectors() and
> > they are all the same thing (i_size of the bdev). Is there a reason for the
> > distinction?
>
> get_capacity/set_capacity are the existing unchanged interfaces that
> work on struct gendisk, and unchanged from what we had before. They also
> have lots of users which makes them kinda awkward to touch.
>
> bdev_nr_sectors is the public interface to query the size for any
> kind of struct block device, to be used by consumers of the block
> device interface.
>
> bdev_set_nr_sectors is a private helper for the partitions core that
> avoids duplicating a bit of code, and works on partitions.
OK, I guess I'll get used to this...
> > > @@ -38,6 +38,16 @@ static void disk_add_events(struct gendisk *disk);
> > > static void disk_del_events(struct gendisk *disk);
> > > static void disk_release_events(struct gendisk *disk);
> > >
> > > +void set_capacity(struct gendisk *disk, sector_t sectors)
> > > +{
> > > + struct block_device *bdev = disk->part0.bdev;
> > > +
> > > + spin_lock(&bdev->bd_size_lock);
> > > + i_size_write(bdev->bd_inode, (loff_t)sectors << SECTOR_SHIFT);
> > > + spin_unlock(&bdev->bd_size_lock);
> >
> > AFAICT bd_size_lock is pointless after these changes so we can just remove
> > it?
>
> I don't think it is, as reuqiring bd_mutex for size updates leads to
> rather awkward lock ordering problems.
OK, let me ask differently: What is bd_size_lock protecting now? Ah, I see,
on 32-bit it is needed to prevent torn writes to i_size, right?
> > > if (capacity != size && capacity != 0 && size != 0) {
> > > char *envp[] = { "RESIZE=1", NULL };
> > >
> > > + pr_info("%s: detected capacity change from %lld to %lld\n",
> > > + disk->disk_name, size, capacity);
> >
> > So we are now missing above message for transitions from / to 0 capacity?
> > Is there any other notification in the kernel log when e.g. media is
> > inserted into a CD-ROM drive? I remember using these messages for detecting
> > that...
>
> True, I guess we should keep the messages for that case at least under
> some circumstances. Let me take a closer look at what could make sense.
>
> > Also what about GENHD_FL_HIDDEN devices? Are we sure we never set capacity
> > for them?
>
> We absolutely set the capacity for them, as we have to. And even use
> this interface. But yes, I think we should skip sending the uevent for
> them.
Also previously we were not printing any messages for hidden devices and
now we do. I'm not sure whether that's intended or not.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
