[PATCH 0/3] fs: minor fs thaw fixes and adjustments

[PATCH 0/3] fs: minor fs thaw fixes and adjustments

[Luis R. Rodriguez]

Here's a few minor fs thaw fixes and adjustments I ran accross
as I started to refresh my development for to use freeze_fs on
suspend/hibernation [0]. These are just prep commits for the real
work, I'm still reviewing feedback and adjusting the code for
that.

I've tested the patches with generic/085 on xfs and found no regressions.

Luis R. Rodriguez (3):
  fs: move documentation for thaw_super() where appropriate
  fs: make thaw_super_locked() really just a helper
  fs: fix corner case race on freeze_bdev() when sb disappears

 fs/block_dev.c |  4 +++-
 fs/super.c     | 39 ++++++++++++++++++++++++++-------------
 2 files changed, 29 insertions(+), 14 deletions(-)

-- 
2.16.3

[PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Luis R. Rodriguez]

On commit 08fdc8a0138a ("buffer.c: call thaw_super during emergency thaw")
Mateusz added thaw_super_locked() and made thaw_super() use it, but
forgot to move the documentation.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/super.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 5fa9a8d8d865..9d0eb5e20a1f 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1532,12 +1532,6 @@ int freeze_super(struct super_block *sb)
 }
 EXPORT_SYMBOL(freeze_super);
 
-/**
- * thaw_super -- unlock filesystem
- * @sb: the super to thaw
- *
- * Unlocks the filesystem and marks it writeable again after freeze_super().
- */
 static int thaw_super_locked(struct super_block *sb)
 {
 	int error;
@@ -1573,6 +1567,12 @@ static int thaw_super_locked(struct super_block *sb)
 	return 0;
 }
 
+/**
+ * thaw_super -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ */
 int thaw_super(struct super_block *sb)
 {
 	down_write(&sb->s_umount);
-- 
2.16.3

Re: [PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Bart Van Assche]

T24gRnJpLCAyMDE4LTA0LTIwIGF0IDE2OjU5IC0wNzAwLCBMdWlzIFIuIFJvZHJpZ3VleiB3cm90
ZToNCj4gKy8qKg0KPiArICogdGhhd19zdXBlciAtLSB1bmxvY2sgZmlsZXN5c3RlbQ0KPiArICog
QHNiOiB0aGUgc3VwZXIgdG8gdGhhdw0KPiArICoNCj4gKyAqIFVubG9ja3MgdGhlIGZpbGVzeXN0
ZW0gYW5kIG1hcmtzIGl0IHdyaXRlYWJsZSBhZ2FpbiBhZnRlciBmcmVlemVfc3VwZXIoKS4NCj4g
KyAqLw0KDQpIYXZlIHlvdSB2ZXJpZmllZCB0aGUgb3V0cHV0IGdlbmVyYXRlZCBieSBzY3JpcHRz
L2tlcm5lbC1kb2M/IExhc3QNCnRpbWUgSSBjaGVja2VkIHRoZSBvdXRwdXQgZm9yIGhlYWRlcnMg
Y29udGFpbmluZyBhIGRvdWJsZSBkYXNoIGxvb2tlZA0Kd2VpcmQuDQoNCkJhcnQuDQoNCg0KDQo=

Re: [PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Bart Van Assche]

On Fri, 2018-04-20 at 16:59 -0700, Luis R. Rodriguez wrote:
> +/**
> + * thaw_super -- unlock filesystem
> + * @sb: the super to thaw
> + *
> + * Unlocks the filesystem and marks it writeable again after freeze_super().
> + */

Have you verified the output generated by scripts/kernel-doc? Last
time I checked the output for headers containing a double dash looked
weird.

Bart.

Re: [PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Luis R. Rodriguez]

On Sat, Apr 21, 2018 at 12:01:31AM +0000, Bart Van Assche wrote:
> On Fri, 2018-04-20 at 16:59 -0700, Luis R. Rodriguez wrote:
> > +/**
> > + * thaw_super -- unlock filesystem
> > + * @sb: the super to thaw
> > + *
> > + * Unlocks the filesystem and marks it writeable again after freeze_super().
> > + */
> 
> Have you verified the output generated by scripts/kernel-doc? Last
> time I checked the output for headers containing a double dash looked
> weird.

No, I just moved the block of kdoc crap. Randy would have this memorized I'm
sure though so I should just fix the kdoc if its bad while at it.

  Luis

Re: [PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Randy Dunlap]

On 04/20/18 17:07, Luis R. Rodriguez wrote:
> On Sat, Apr 21, 2018 at 12:01:31AM +0000, Bart Van Assche wrote:
>> On Fri, 2018-04-20 at 16:59 -0700, Luis R. Rodriguez wrote:
>>> +/**
>>> + * thaw_super -- unlock filesystem
>>> + * @sb: the super to thaw
>>> + *
>>> + * Unlocks the filesystem and marks it writeable again after freeze_super().
>>> + */
>>
>> Have you verified the output generated by scripts/kernel-doc? Last
>> time I checked the output for headers containing a double dash looked
>> weird.
> 
> No, I just moved the block of kdoc crap. Randy would have this memorized I'm
> sure though so I should just fix the kdoc if its bad while at it.
> 
>   Luis
> 

"--" does sound odd.  I've never seen it used on purpose.
FWIW, I just go by what is in Documentation/doc-guide/kernel-doc.rst:

Function documentation
----------------------

The general format of a function and function-like macro kernel-doc comment is::

  /**
   * function_name() - Brief description of function.
   * @arg1: Describe the first argument.
   * @arg2: Describe the second argument.
   *        One can provide multiple line descriptions
   *        for arguments.



-- 
~Randy

Re: [PATCH 1/3] fs: move documentation for thaw_super() where appropriate

[Jan Kara]

On Fri 20-04-18 16:59:02, Luis R. Rodriguez wrote:
> On commit 08fdc8a0138a ("buffer.c: call thaw_super during emergency thaw")
> Mateusz added thaw_super_locked() and made thaw_super() use it, but
> forgot to move the documentation.
> 
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Looks good (modulo the -- which is probably worth fixing when touching the
comment anyway). You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 5fa9a8d8d865..9d0eb5e20a1f 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1532,12 +1532,6 @@ int freeze_super(struct super_block *sb)
>  }
>  EXPORT_SYMBOL(freeze_super);
>  
> -/**
> - * thaw_super -- unlock filesystem
> - * @sb: the super to thaw
> - *
> - * Unlocks the filesystem and marks it writeable again after freeze_super().
> - */
>  static int thaw_super_locked(struct super_block *sb)
>  {
>  	int error;
> @@ -1573,6 +1567,12 @@ static int thaw_super_locked(struct super_block *sb)
>  	return 0;
>  }
>  
> +/**
> + * thaw_super -- unlock filesystem
> + * @sb: the super to thaw
> + *
> + * Unlocks the filesystem and marks it writeable again after freeze_super().
> + */
>  int thaw_super(struct super_block *sb)
>  {
>  	down_write(&sb->s_umount);
> -- 
> 2.16.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 2/3] fs: make thaw_super_locked() really just a helper

[Luis R. Rodriguez]

thaw_super_locked() was added via commit 08fdc8a0138a ("buffer.c: call
thaw_super during emergency thaw") merged on v4.17 to help with the
ability so that the caller can take charge of handling the s_umount lock,
however, it has left all* of the failure handling including unlocking
lock of s_umount inside thaw_super_locked().

This does not make thaw_super_locked() flexible. For instance we may
later want to use it with the abilty to handle unfolding of the locks
ourselves.

Change thaw_super_locked() to really just be a helper, and let the
callers deal with all the error handling.

This commit introeuces no functional changes.

Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/super.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 9d0eb5e20a1f..82bc74a16f06 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -937,10 +937,15 @@ void emergency_remount(void)
 
 static void do_thaw_all_callback(struct super_block *sb)
 {
+	int error;
+
 	down_write(&sb->s_umount);
 	if (sb->s_root && sb->s_flags & MS_BORN) {
 		emergency_thaw_bdev(sb);
-		thaw_super_locked(sb);
+		error = thaw_super_locked(sb);
+		if (error)
+			up_write(&sb->s_umount);
+		deactivate_locked_super(sb);
 	} else {
 		up_write(&sb->s_umount);
 	}
@@ -1532,14 +1537,13 @@ int freeze_super(struct super_block *sb)
 }
 EXPORT_SYMBOL(freeze_super);
 
+/* Caller takes the sb->s_umount rw_semaphore lock and handles active count */
 static int thaw_super_locked(struct super_block *sb)
 {
 	int error;
 
-	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
-		up_write(&sb->s_umount);
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
 		return -EINVAL;
-	}
 
 	if (sb_rdonly(sb)) {
 		sb->s_writers.frozen = SB_UNFROZEN;
@@ -1554,7 +1558,6 @@ static int thaw_super_locked(struct super_block *sb)
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			up_write(&sb->s_umount);
 			return error;
 		}
 	}
@@ -1563,7 +1566,6 @@ static int thaw_super_locked(struct super_block *sb)
 	sb_freeze_unlock(sb);
 out:
 	wake_up(&sb->s_writers.wait_unfrozen);
-	deactivate_locked_super(sb);
 	return 0;
 }
 
@@ -1575,7 +1577,18 @@ static int thaw_super_locked(struct super_block *sb)
  */
 int thaw_super(struct super_block *sb)
 {
+	int error;
+
 	down_write(&sb->s_umount);
-	return thaw_super_locked(sb);
+	error = thaw_super_locked(sb);
+	if (error) {
+		up_write(&sb->s_umount);
+		goto out;
+	}
+
+	deactivate_locked_super(sb);
+
+out:
+	return error;
 }
 EXPORT_SYMBOL(thaw_super);
-- 
2.16.3

Re: [PATCH 2/3] fs: make thaw_super_locked() really just a helper

[Jan Kara]

On Fri 20-04-18 16:59:03, Luis R. Rodriguez wrote:
> thaw_super_locked() was added via commit 08fdc8a0138a ("buffer.c: call
> thaw_super during emergency thaw") merged on v4.17 to help with the
> ability so that the caller can take charge of handling the s_umount lock,
> however, it has left all* of the failure handling including unlocking
> lock of s_umount inside thaw_super_locked().
> 
> This does not make thaw_super_locked() flexible. For instance we may
> later want to use it with the abilty to handle unfolding of the locks
> ourselves.
> 
> Change thaw_super_locked() to really just be a helper, and let the
> callers deal with all the error handling.

And do you have use for the new thaw_super_locked()? Because the new
semantics with having to deal with deactivate_locked_super() does not seem
ideal either so as a standalone cleanup patch it does not look too
useful.
 
> This commit introeuces no functional changes.
		^^^ introduces
 
								Honza

> ---
>  fs/super.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 9d0eb5e20a1f..82bc74a16f06 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -937,10 +937,15 @@ void emergency_remount(void)
>  
>  static void do_thaw_all_callback(struct super_block *sb)
>  {
> +	int error;
> +
>  	down_write(&sb->s_umount);
>  	if (sb->s_root && sb->s_flags & MS_BORN) {
>  		emergency_thaw_bdev(sb);
> -		thaw_super_locked(sb);
> +		error = thaw_super_locked(sb);
> +		if (error)
> +			up_write(&sb->s_umount);
> +		deactivate_locked_super(sb);
>  	} else {
>  		up_write(&sb->s_umount);
>  	}
> @@ -1532,14 +1537,13 @@ int freeze_super(struct super_block *sb)
>  }
>  EXPORT_SYMBOL(freeze_super);
>  
> +/* Caller takes the sb->s_umount rw_semaphore lock and handles active count */
>  static int thaw_super_locked(struct super_block *sb)
>  {
>  	int error;
>  
> -	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
> -		up_write(&sb->s_umount);
> +	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
>  		return -EINVAL;
> -	}
>  
>  	if (sb_rdonly(sb)) {
>  		sb->s_writers.frozen = SB_UNFROZEN;
> @@ -1554,7 +1558,6 @@ static int thaw_super_locked(struct super_block *sb)
>  			printk(KERN_ERR
>  				"VFS:Filesystem thaw failed\n");
>  			lockdep_sb_freeze_release(sb);
> -			up_write(&sb->s_umount);
>  			return error;
>  		}
>  	}
> @@ -1563,7 +1566,6 @@ static int thaw_super_locked(struct super_block *sb)
>  	sb_freeze_unlock(sb);
>  out:
>  	wake_up(&sb->s_writers.wait_unfrozen);
> -	deactivate_locked_super(sb);
>  	return 0;
>  }
>  
> @@ -1575,7 +1577,18 @@ static int thaw_super_locked(struct super_block *sb)
>   */
>  int thaw_super(struct super_block *sb)
>  {
> +	int error;
> +
>  	down_write(&sb->s_umount);
> -	return thaw_super_locked(sb);
> +	error = thaw_super_locked(sb);
> +	if (error) {
> +		up_write(&sb->s_umount);
> +		goto out;
> +	}
> +
> +	deactivate_locked_super(sb);
> +
> +out:
> +	return error;
>  }
>  EXPORT_SYMBOL(thaw_super);
> -- 
> 2.16.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH 3/3] fs: fix corner case race on freeze_bdev() when sb disappears

[Luis R. Rodriguez]

freeze_bdev() will bail but leave the bd_fsfreeze_count incremented
if the get_active_super() does not find the superblock on our
super_blocks list to match.

This issue has been present since v2.6.29 during the introduction of the
ioctl_fsfreeze() and ioctl_fsthaw() via commit fcccf502540e3 ("filesystem
freeze: implement generic freeze feature").

I am not aware of any existing races which have triggered this
situation, however, if it does trigger it could mean leaving a
superblock with bd_fsfreeze_count always positive.

Fixes: fcccf502540e3 ("filesystem freeze: implement generic freeze feature")
Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>
---
 fs/block_dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/block_dev.c b/fs/block_dev.c
index b54966679833..7a532aa58c07 100644
--- a/fs/block_dev.c
+++ b/fs/block_dev.c
@@ -507,8 +507,10 @@ struct super_block *freeze_bdev(struct block_device *bdev)
 	}
 
 	sb = get_active_super(bdev);
-	if (!sb)
+	if (!sb) {
+		bdev->bd_fsfreeze_count--;
 		goto out;
+	}
 	if (sb->s_op->freeze_super)
 		error = sb->s_op->freeze_super(sb);
 	else
-- 
2.16.3

Re: [PATCH 3/3] fs: fix corner case race on freeze_bdev() when sb disappears

[Jan Kara]

On Fri 20-04-18 16:59:04, Luis R. Rodriguez wrote:
> freeze_bdev() will bail but leave the bd_fsfreeze_count incremented
> if the get_active_super() does not find the superblock on our
> super_blocks list to match.
> 
> This issue has been present since v2.6.29 during the introduction of the
> ioctl_fsfreeze() and ioctl_fsthaw() via commit fcccf502540e3 ("filesystem
> freeze: implement generic freeze feature").
> 
> I am not aware of any existing races which have triggered this
> situation, however, if it does trigger it could mean leaving a
> superblock with bd_fsfreeze_count always positive.
> 
> Fixes: fcccf502540e3 ("filesystem freeze: implement generic freeze feature")
> Signed-off-by: Luis R. Rodriguez <mcgrof@kernel.org>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/block_dev.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/block_dev.c b/fs/block_dev.c
> index b54966679833..7a532aa58c07 100644
> --- a/fs/block_dev.c
> +++ b/fs/block_dev.c
> @@ -507,8 +507,10 @@ struct super_block *freeze_bdev(struct block_device *bdev)
>  	}
>  
>  	sb = get_active_super(bdev);
> -	if (!sb)
> +	if (!sb) {
> +		bdev->bd_fsfreeze_count--;
>  		goto out;
> +	}
>  	if (sb->s_op->freeze_super)
>  		error = sb->s_op->freeze_super(sb);
>  	else
> -- 
> 2.16.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/3] fs: minor fs thaw fixes and adjustments

[Luis R. Rodriguez]

On Fri, Apr 20, 2018 at 04:59:01PM -0700, Luis R. Rodriguez wrote:
> Here's a few minor fs thaw fixes and adjustments I ran accross
> as I started to refresh my development for to use freeze_fs on
> suspend/hibernation [0]. These are just prep commits for the real
> work, I'm still reviewing feedback and adjusting the code for
> that.

And I forgot to provide the URL, for those that missed the old series
I was working on:

[0] https://lkml.kernel.org/r/20171129232356.28296-1-mcgrof@kernel.org

  Luis
> 
> I've tested the patches with generic/085 on xfs and found no regressions.
> 
> Luis R. Rodriguez (3):
>   fs: move documentation for thaw_super() where appropriate
>   fs: make thaw_super_locked() really just a helper
>   fs: fix corner case race on freeze_bdev() when sb disappears
> 
>  fs/block_dev.c |  4 +++-
>  fs/super.c     | 39 ++++++++++++++++++++++++++-------------
>  2 files changed, 29 insertions(+), 14 deletions(-)
> 
> -- 
> 2.16.3
> 
> 

-- 
Do not panic