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

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

[Jan Kara]

Hello,

I've been debugging for quite a long time strange issues with encrypted DVDs
not being readable on Linux (bko#194965). In the end I've tracked down the
problem to the fact that block device size is not updated when the media is
inserted in case the DVD device is already open. This is IMO a bug in block
device code as the size gets properly update in case the device has partition
scanning enabled.  The following series fixes the problem by refreshing disk
size on each open even for devices with partition scanning disabled.

								Honza

[PATCH 1/2] bdev: Factor out bdev revalidation into a common helper

[Jan Kara]

Factor out code handling revalidation of bdev on disk change into a
common helper.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/block_dev.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index 9c073dbdc1b0..88c6d35ec71d 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -1512,6 +1512,14 @@ EXPORT_SYMBOL(bd_set_size);
 
 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);
+}
+
 /*
  * bd_mutex locking:
  *
@@ -1594,12 +1602,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 			 * The latter is necessary to prevent ghost
 			 * partitions on a removed medium.
 			 */
-			if (bdev->bd_invalidated) {
-				if (!ret)
-					rescan_partitions(disk, bdev);
-				else if (ret == -ENOMEDIUM)
-					invalidate_partitions(disk, bdev);
-			}
+			if (bdev->bd_invalidated &&
+			    (!ret || ret == -ENOMEDIUM))
+				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
 
 			if (ret)
 				goto out_clear;
@@ -1632,12 +1637,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
 			if (bdev->bd_disk->fops->open)
 				ret = bdev->bd_disk->fops->open(bdev, mode);
 			/* the same as first opener case, read comment there */
-			if (bdev->bd_invalidated) {
-				if (!ret)
-					rescan_partitions(bdev->bd_disk, bdev);
-				else if (ret == -ENOMEDIUM)
-					invalidate_partitions(bdev->bd_disk, bdev);
-			}
+			if (bdev->bd_invalidated &&
+			    (!ret || ret == -ENOMEDIUM))
+				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
 			if (ret)
 				goto out_unlock_bdev;
 		}
-- 
2.16.4

Re: [PATCH 1/2] bdev: Factor out bdev revalidation into a common helper

[Damien Le Moal]

On 2019/10/21 17:38, Jan Kara wrote:
> Factor out code handling revalidation of bdev on disk change into a
> common helper.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/block_dev.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index 9c073dbdc1b0..88c6d35ec71d 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -1512,6 +1512,14 @@ EXPORT_SYMBOL(bd_set_size);
>  
>  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);
> +}
> +
>  /*
>   * bd_mutex locking:
>   *
> @@ -1594,12 +1602,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>  			 * The latter is necessary to prevent ghost
>  			 * partitions on a removed medium.
>  			 */
> -			if (bdev->bd_invalidated) {
> -				if (!ret)
> -					rescan_partitions(disk, bdev);
> -				else if (ret == -ENOMEDIUM)
> -					invalidate_partitions(disk, bdev);
> -			}
> +			if (bdev->bd_invalidated &&
> +			    (!ret || ret == -ENOMEDIUM))
> +				bdev_disk_changed(bdev, ret == -ENOMEDIUM);

This is a little confusing since from its name, bdev_disk_changed() seem
to imply that a new disk is present but this is called only if bdev is
invalidated... What about calling this simply bdev_revalidate_disk(),
since rescan_partitions() will call the disk revalidate method.

Also, it seems to me that this function could be used without the
complex "if ()" condition by slightly modifying it:

static void bdev_revalidate_disk(struct block_device *bdev,
			         bool invalidate)
{
	if (bdev->bd_invalidated && invalidate)
		invalidate_partitions(bdev->bd_disk, bdev);
	else
		rescan_partitions(bdev->bd_disk, bdev);
}

Otherwise, this all looks fine to me.

>  
>  			if (ret)
>  				goto out_clear;
> @@ -1632,12 +1637,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>  			if (bdev->bd_disk->fops->open)
>  				ret = bdev->bd_disk->fops->open(bdev, mode);
>  			/* the same as first opener case, read comment there */
> -			if (bdev->bd_invalidated) {
> -				if (!ret)
> -					rescan_partitions(bdev->bd_disk, bdev);
> -				else if (ret == -ENOMEDIUM)
> -					invalidate_partitions(bdev->bd_disk, bdev);
> -			}
> +			if (bdev->bd_invalidated &&
> +			    (!ret || ret == -ENOMEDIUM))
> +				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
>  			if (ret)
>  				goto out_unlock_bdev;
>  		}
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] bdev: Factor out bdev revalidation into a common helper

[Jan Kara]

On Tue 22-10-19 07:58:08, Damien Le Moal wrote:
> On 2019/10/21 17:38, Jan Kara wrote:
> > Factor out code handling revalidation of bdev on disk change into a
> > common helper.
> > 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/block_dev.c | 26 ++++++++++++++------------
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/block_dev.c b/fs/block_dev.c
> > index 9c073dbdc1b0..88c6d35ec71d 100644
> > --- a/fs/block_dev.c
> > +++ b/fs/block_dev.c
> > @@ -1512,6 +1512,14 @@ EXPORT_SYMBOL(bd_set_size);
> >  
> >  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);
> > +}
> > +
> >  /*
> >   * bd_mutex locking:
> >   *
> > @@ -1594,12 +1602,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
> >  			 * The latter is necessary to prevent ghost
> >  			 * partitions on a removed medium.
> >  			 */
> > -			if (bdev->bd_invalidated) {
> > -				if (!ret)
> > -					rescan_partitions(disk, bdev);
> > -				else if (ret == -ENOMEDIUM)
> > -					invalidate_partitions(disk, bdev);
> > -			}
> > +			if (bdev->bd_invalidated &&
> > +			    (!ret || ret == -ENOMEDIUM))
> > +				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
> 
> This is a little confusing since from its name, bdev_disk_changed() seem
> to imply that a new disk is present but this is called only if bdev is
> invalidated... What about calling this simply bdev_revalidate_disk(),
> since rescan_partitions() will call the disk revalidate method.

Honestly, the whole disk revalidation code is confusing to me :) I had to
draw a graph of which function calls which to understand what's going on in
that code and I think it could really use some refactoring. But that's
besides current point :)

Your "only if bdev is invalidated" is true but not actually a full story.
->bd_invalidated effectively gets set only through check_disk_change(). All
other places that end up calling flush_disk() clear bd_invalidated shortly
afterwards. So the function I've created is a direct counterpart to
check_disk_change() that just needs to happen after the device is
successfully open. I wanted to express that in the name - hence
bdev_disk_changed(). So yes, bdev_disk_changed() should be called exactly
when the new disk is present. It is bd_invalidated that is actually
misnamed.

Now I'm not really tied to bdev_disk_changed(). bdev_revalidate_disk()
seems a bit confusing to me though because the disk has actually been
already revalidated in check_disk_change() and the function won't
revalidate the disk for devices with partition scan disabled.

> Also, it seems to me that this function could be used without the
> complex "if ()" condition by slightly modifying it:
> 
> static void bdev_revalidate_disk(struct block_device *bdev,
> 			         bool invalidate)
> {
> 	if (bdev->bd_invalidated && invalidate)
> 		invalidate_partitions(bdev->bd_disk, bdev);
> 	else
> 		rescan_partitions(bdev->bd_disk, bdev);
> }
> 
> Otherwise, this all looks fine to me.

Well, but you don't want to call rescan_partitions() if bd_invalidated is
not set. But yes, we could move bd_invalidated check into
bdev_disk_changed(), but then it would seem less clear why this function is
getting called. This ties somewhat to the discussion above. Hum, I guess if
we call the function just bdev_revalidate(), the name won't be confusing
and it would then make sense to move the bd_invalidated condition in there.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 1/2] bdev: Factor out bdev revalidation into a common helper

[Damien Le Moal]

On 2019/10/22 18:15, Jan Kara wrote:
> On Tue 22-10-19 07:58:08, Damien Le Moal wrote:
>> On 2019/10/21 17:38, Jan Kara wrote:
>>> Factor out code handling revalidation of bdev on disk change into a
>>> common helper.
>>>
>>> Signed-off-by: Jan Kara <jack@suse.cz>
>>> ---
>>>  fs/block_dev.c | 26 ++++++++++++++------------
>>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/fs/block_dev.c b/fs/block_dev.c
>>> index 9c073dbdc1b0..88c6d35ec71d 100644
>>> --- a/fs/block_dev.c
>>> +++ b/fs/block_dev.c
>>> @@ -1512,6 +1512,14 @@ EXPORT_SYMBOL(bd_set_size);
>>>  
>>>  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);
>>> +}
>>> +
>>>  /*
>>>   * bd_mutex locking:
>>>   *
>>> @@ -1594,12 +1602,9 @@ static int __blkdev_get(struct block_device *bdev, fmode_t mode, int for_part)
>>>  			 * The latter is necessary to prevent ghost
>>>  			 * partitions on a removed medium.
>>>  			 */
>>> -			if (bdev->bd_invalidated) {
>>> -				if (!ret)
>>> -					rescan_partitions(disk, bdev);
>>> -				else if (ret == -ENOMEDIUM)
>>> -					invalidate_partitions(disk, bdev);
>>> -			}
>>> +			if (bdev->bd_invalidated &&
>>> +			    (!ret || ret == -ENOMEDIUM))
>>> +				bdev_disk_changed(bdev, ret == -ENOMEDIUM);
>>
>> This is a little confusing since from its name, bdev_disk_changed() seem
>> to imply that a new disk is present but this is called only if bdev is
>> invalidated... What about calling this simply bdev_revalidate_disk(),
>> since rescan_partitions() will call the disk revalidate method.
> 
> Honestly, the whole disk revalidation code is confusing to me :) I had to
> draw a graph of which function calls which to understand what's going on in
> that code and I think it could really use some refactoring. But that's
> besides current point :)

OK. I guess I got confused too...

> 
> Your "only if bdev is invalidated" is true but not actually a full story.
> ->bd_invalidated effectively gets set only through check_disk_change(). All
> other places that end up calling flush_disk() clear bd_invalidated shortly
> afterwards. So the function I've created is a direct counterpart to
> check_disk_change() that just needs to happen after the device is
> successfully open. I wanted to express that in the name - hence
> bdev_disk_changed(). So yes, bdev_disk_changed() should be called exactly
> when the new disk is present. It is bd_invalidated that is actually
> misnamed.

Got it.

> 
> Now I'm not really tied to bdev_disk_changed(). bdev_revalidate_disk()
> seems a bit confusing to me though because the disk has actually been
> already revalidated in check_disk_change() and the function won't
> revalidate the disk for devices with partition scan disabled.>
>> Also, it seems to me that this function could be used without the
>> complex "if ()" condition by slightly modifying it:
>>
>> static void bdev_revalidate_disk(struct block_device *bdev,
>> 			         bool invalidate)
>> {
>> 	if (bdev->bd_invalidated && invalidate)
>> 		invalidate_partitions(bdev->bd_disk, bdev);
>> 	else
>> 		rescan_partitions(bdev->bd_disk, bdev);
>> }
>>
>> Otherwise, this all looks fine to me.
> 
> Well, but you don't want to call rescan_partitions() if bd_invalidated is
> not set. But yes, we could move bd_invalidated check into
> bdev_disk_changed(), but then it would seem less clear why this function is
> getting called. This ties somewhat to the discussion above. Hum, I guess if
> we call the function just bdev_revalidate(), the name won't be confusing
> and it would then make sense to move the bd_invalidated condition in there.

Indeed, we don't want another partition scan. Missed that one.
For the function name, since the goal is to revalidate only the bdev
capacity, what about bdev_revalidate_capacity() then ? But looking at
the code and seeing the partitions functions calls does not clarify
things though. Oh well, keep the name you proposed and we can cleanup
everything with a refactor :)

Cheers.

-- 
Damien Le Moal
Western Digital Research

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

[Jan Kara]

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);
+	} else {
+		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
+		bdev->bd_invalidated = 0;
+	}
 }
 
 /*
-- 
2.16.4

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

[Guoqing Jiang]

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.

Thanks,
Guoqing

> +	} else {
> +		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
> +		bdev->bd_invalidated = 0;
> +	}
>   }
>   
>   /*

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

[Jan Kara]

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...

								Honza

> > +	} else {
> > +		check_disk_size_change(bdev->bd_disk, bdev, !invalidate);
> > +		bdev->bd_invalidated = 0;
> > +	}
> >   }
> >   /*
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Guoqing Jiang]

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?

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;
	}
}

Thanks,
Guoqing

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

[Johannes Thumshirn]

On 21/10/2019 11:27, Guoqing Jiang wrote:
>>>>    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.

The above is changed in the new bdev_disk_changed() function. Patch 1/2
factors out the pattern and this patch changes the behaviour



-- 
Johannes Thumshirn                            SUSE Labs Filesystems
jthumshirn@suse.de                                +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

[Guoqing Jiang]

On 10/21/19 11:36 AM, Johannes Thumshirn wrote:
> On 21/10/2019 11:27, Guoqing Jiang wrote:
>>>>>     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.
> The above is changed in the new bdev_disk_changed() function. Patch 1/2
> factors out the pattern and this patch changes the behaviour
>

Oops, yes, sorry for the noise.

Thanks,
Guoqing

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

[Jan Kara]

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().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Guoqing Jiang]

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

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

[Jens Axboe]

On 10/21/19 2:37 AM, Jan Kara wrote:
> Hello,
> 
> I've been debugging for quite a long time strange issues with encrypted DVDs
> not being readable on Linux (bko#194965). In the end I've tracked down the
> problem to the fact that block device size is not updated when the media is
> inserted in case the DVD device is already open. This is IMO a bug in block
> device code as the size gets properly update in case the device has partition
> scanning enabled.  The following series fixes the problem by refreshing disk
> size on each open even for devices with partition scanning disabled.

It's really confusing to have different behavior for partition vs whole device.
This series looks good to me, the size change code is really hard to follow.

I don't see any serious objections here, I'm going to queue this up for
5.4.

-- 
Jens Axboe

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

[Jan Kara]

On Sun 03-11-19 07:53:10, Jens Axboe wrote:
> On 10/21/19 2:37 AM, Jan Kara wrote:
> > Hello,
> > 
> > I've been debugging for quite a long time strange issues with encrypted DVDs
> > not being readable on Linux (bko#194965). In the end I've tracked down the
> > problem to the fact that block device size is not updated when the media is
> > inserted in case the DVD device is already open. This is IMO a bug in block
> > device code as the size gets properly update in case the device has partition
> > scanning enabled.  The following series fixes the problem by refreshing disk
> > size on each open even for devices with partition scanning disabled.
> 
> It's really confusing to have different behavior for partition vs whole device.
> This series looks good to me, the size change code is really hard to follow.
> 
> I don't see any serious objections here, I'm going to queue this up for
> 5.4.

Thanks Jens! I'll look into refactoring the size change / revalidation code
so that it's easier to understand what's going on...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Christoph Hellwig]

On Mon, Nov 04, 2019 at 09:08:47AM +0100, Jan Kara wrote:
> Thanks Jens! I'll look into refactoring the size change / revalidation code
> so that it's easier to understand what's going on...

I actualy have a series for this.  I've started rebasing it on top
of you work and will need to do some testing.  My current WIP is here:

http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/partition-cleanup

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

[Jan Kara]

On Mon 04-11-19 15:48:41, Christoph Hellwig wrote:
> On Mon, Nov 04, 2019 at 09:08:47AM +0100, Jan Kara wrote:
> > Thanks Jens! I'll look into refactoring the size change / revalidation code
> > so that it's easier to understand what's going on...
> 
> I actualy have a series for this.  I've started rebasing it on top
> of you work and will need to do some testing.  My current WIP is here:
> 
> http://git.infradead.org/users/hch/block.git/shortlog/refs/heads/partition-cleanup

Cool. Thanks for that! Skimming over the series it looks good to me. The
only remaining thing I wanted to look into is bdev->bd_invalidated
handling. Because the only thing it actually indicates in practice is that
DISK_EVENT_MEDIA_CHANGE was set in check_disk_change(). All the other call
chains end up clearing bdev->bd_invalidated before it has any effect. And
that just looks very cryptic to me... So my plan was to at least move the
setting of bdev->bd_invalidated to check_disk_change() and rename it to
something saner if I cannot come up with anything better for propagating
the information from check_disk_change() to __blkdev_get().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR