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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
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/[email protected]
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
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
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
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 <[email protected]>
Looks good (modulo the -- which is probably worth fixing when touching the
comment anyway). You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
Looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
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 <[email protected]>
SUSE Labs, CR