From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dan Williams Subject: Re: Time to make dynamically allocated devt the default for scsi disks? Date: Fri, 12 Aug 2016 17:29:30 -0700 Message-ID: References: <1471047451.2407.95.camel@HansenPartnership.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <1471047451.2407.95.camel@HansenPartnership.com> Sender: linux-block-owner@vger.kernel.org To: James Bottomley Cc: linux-block@vger.kernel.org, linux-scsi , Jens Axboe , "Martin K. Petersen" , Christoph Hellwig , Tejun Heo , Dave Hansen List-Id: linux-scsi@vger.kernel.org On Fri, Aug 12, 2016 at 5:17 PM, James Bottomley wrote: > On Fri, 2016-08-12 at 14:29 -0700, Dan Williams wrote: >> Before spending effort trying to flush the destruction of old bdi >> instances before new ones are registered, is it rather time to >> complete the conversion of sd to only use dynamically allocated devt? > > Do we have to go that far? Surely your fix is extensible: the only > reason it doesn't work for us is that the gendisk holds the parent > without a reference, so we can free the SCSI device before its child > gendisk (good job no-one actually uses gendisk->parent after we've > released it ...). If we fix that it would mean SCSI can't release the > sdev until after the queue is dead and the bdi namespace released, so > isn't something like this the easy fix? > > James > > --- > > diff --git a/block/genhd.c b/block/genhd.c > index fcd6d4f..54ae4ae 100644 > --- a/block/genhd.c > +++ b/block/genhd.c > @@ -514,7 +514,7 @@ static void register_disk(struct device *parent, struct gendisk *disk) > struct hd_struct *part; > int err; > > - ddev->parent = parent; > + ddev->parent = get_device(parent); > > dev_set_name(ddev, "%s", disk->disk_name); > > @@ -1144,6 +1144,7 @@ static void disk_release(struct device *dev) > hd_free_part(&disk->part0); > if (disk->queue) > blk_put_queue(disk->queue); > + put_device(dev->parent); > kfree(disk); > } > struct class block_class = { Looks ok at first glance to me. We do hold a reference on the parent device, but it gets dropped at device_unregister() time and this moves it out to the final put. However, this does leave static devt block-device-drivers that register a disk without a parent device susceptible to the race... I think those exist given all the drivers still using add_disk() after commit 52c44d93c26f "block: remove ->driverfs_dev".