On Thu 09-02-23 17:32:45, Yu Kuai wrote:
> 在 2023/02/09 17:04, Jan Kara 写道:
> > On Wed 08-02-23 22:13:02, Yu Kuai wrote:
> > > Hi,
> > >
> > > 在 2023/02/08 20:02, Jan Kara 写道:
> > > >
> > > > After some thought I don't like opencoding blkdev_get_by_dev() in disk_scan
> > > > partitions. But I agree Christoph's approach with blkdev_get_whole() does
> > > > not quite work either. We could propagate holder/owner into
> > > > blkdev_get_whole() to fix Christoph's check but still we are left with a
> > > > question what to do with GD_NEED_PART_SCAN set bit when we get into
> > > > blkdev_get_whole() and find out we are not elligible to rescan partitions.
> > > > Because then some exclusive opener later might be caught by surprise when
> > > > the partition rescan happens due to this bit being set from the past failed
> > > > attempt to rescan.
> > > >
> > > > So what we could do is play a similar trick as we do in the loop device and
> > > > do in disk_scan_partitions():
> > > >
> > > > /*
> > > > * If we don't hold exclusive handle for the device, upgrade to it
> > > > * here to avoid changing partitions under exclusive owner.
> > > > */
> > > > if (!(mode & FMODE_EXCL)) {
> > > This is not necessary, all the caller make sure FMODE_EXCL is not set.
> >
> > Yes, but we need to propagate it correctly from blkdev_common_ioctl() now,
> > exactly so that ioctl does not fail if you exclusively opened the device as
> > you realized below :)
>
> Ok, I get it that you want to pass in FMODE_EXCL from ioctl(). But I'm
> not sure let others fail to open device extl is a good idea.
Other exclusive openers will not fail. They will block until we call
bd_abort_claiming() after the partitions have been reread.
> I still prefer to open code blkdev_get_by_dev(), because many operations
> is not necessary here. And this way, we can clear GD_NEED_PART_SCAN
> inside open_mutex if rescan failed.
I understand many operations are not needed there but this is not a hot
path and leaking of bdev internal details into genhd.c is not a good
practice either (e.g. you'd have to make blkdev_get_whole() extern).
We could create a special helper for partition rescan in block/bdev.c to
hide the details but think that bd_start_claiming() - bd_abort_claiming()
trick is the simplest solution to temporarily grab exclusive ownership if
we don't have it.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
