Re: [PATCH 2/2] bdev: Refresh bdev size for disks without partitioning

From: Guoqing Jiang <guoqing.jiang@cloud.ionos.com>
To: Jan Kara <jack@suse.cz>
Cc: Jens Axboe <axboe@kernel.dk>, linux-block@vger.kernel.org
Subject: Re: [PATCH 2/2] bdev: Refresh bdev size for disks without partitioning
Date: Mon, 21 Oct 2019 11:43:03 +0200	[thread overview]
Message-ID: <5d3ea6b2-61c1-fd79-39e9-0b294b8b2358@cloud.ionos.com> (raw)
In-Reply-To: <20191021094016.GD17810@quack2.suse.cz>

On 10/21/19 11:40 AM, Jan Kara wrote:
> On Mon 21-10-19 11:27:36, Guoqing Jiang wrote:
>>
>> On 10/21/19 11:12 AM, Jan Kara wrote:
>>> On Mon 21-10-19 10:49:54, Guoqing Jiang wrote:
>>>> On 10/21/19 10:38 AM, Jan Kara wrote:
>>>>> Currently, block device size in not updated on second and further open
>>>>> for block devices where partition scan is disabled. This is particularly
>>>>> annoying for example for DVD drives as that means block device size does
>>>>> not get updated once the media is inserted into a drive if the device is
>>>>> already open when inserting the media. This is actually always the case
>>>>> for example when pktcdvd is in use.
>>>>>
>>>>> Fix the problem by revalidating block device size on every open even for
>>>>> devices with partition scan disabled.
>>>>>
>>>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>>>> ---
>>>>>     fs/block_dev.c | 19 ++++++++++---------
>>>>>     1 file changed, 10 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>>>> index 88c6d35ec71d..d612468ee66b 100644
>>>>> --- a/fs/block_dev.c
>>>>> +++ b/fs/block_dev.c
>>>>> @@ -1403,11 +1403,7 @@ static void flush_disk(struct block_device *bdev, bool kill_dirty)
>>>>>     		       "resized disk %s\n",
>>>>>     		       bdev->bd_disk ? bdev->bd_disk->disk_name : "");
>>>>>     	}
>>>>> -
>>>>> -	if (!bdev->bd_disk)
>>>>> -		return;
>>>>> -	if (disk_part_scan_enabled(bdev->bd_disk))
>>>>> -		bdev->bd_invalidated = 1;
>>>>> +	bdev->bd_invalidated = 1;
>>>>>     }
>>>>>     /**
>>>>> @@ -1514,10 +1510,15 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part);
>>>>>     static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>>>>>     {
>>>>> -	if (invalidate)
>>>>> -		invalidate_partitions(bdev->bd_disk, bdev);
>>>>> -	else
>>>>> -		rescan_partitions(bdev->bd_disk, bdev);
>>>>> +	if (disk_part_scan_enabled(bdev->bd_disk)) {
>>>>> +		if (invalidate)
>>>>> +			invalidate_partitions(bdev->bd_disk, bdev);
>>>>> +		else
>>>>> +			rescan_partitions(bdev->bd_disk, bdev);
>>>> Maybe use the new common helper to replace above.
>>> What do you mean exactly? Because there's only this place that has the code
>>> pattern here...
>> The above looks same as the new function.
>>
>> +static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>> +{
>> +	if (invalidate)
>> +		invalidate_partitions(bdev->bd_disk, bdev);
>> +	else
>> +		rescan_partitions(bdev->bd_disk, bdev);
>> +}
>>
>> So maybe just call it in the 'if' branch, am I misunderstand something?
> I think you misparsed the diff ;) The code that you mention as duplicated
> just gets reindented by the patch.
>
>> static void bdev_disk_changed(struct block_device *bdev, bool invalidate)
>> {
>> 	if (disk_part_scan_enabled(bdev->bd_disk))	
>> 		bdev_disk_changed(bdev, invalidate)
>> 	else {
>> 		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
>> 		bdev->bd_invalidated = 0;
>> 	}
>> }
> This has infinite recursion in it - you call bdev_disk_changed() from
> bdev_disk_changed().

Right :-[, I need some coffee to refresh, sorry for the noise.

BRs,
Guoqing