[PATCH] btrfs: add dmesg output when mounting and unmounting

[PATCH] btrfs: add dmesg output when mounting and unmounting

[Qu Wenruo]

There is a feature request to add dmesg output when unmounting a btrfs.

There are several alternative methods to do the same thing, but with
their problems:

- Use eBPF to watch btrfs_put_super()/open_ctree()
  Not end user friendly, they have to dip their head into the source
  code.

- Watch for /sys/fs/<uuid>/
  This is way more simpler, but still requires some simple device -> uuid
  lookups.
  And a script needs to use inotify to watch /sys/fs/.

Compared to all these, directly outputting the information into dmesg
would be the most simple one, with both device and UUID included.

And since we're here, also add the output when mounting a btrfs, to keep
the dmesg paired.

Now mounting a btrfs with all default mkfs options would look like this:

[   81.906566] BTRFS info (device dm-8): mounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
[   81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
[   81.908258] BTRFS info (device dm-8): using free space tree
[   81.912644] BTRFS info (device dm-8): auto enabling async discard
[   81.913277] BTRFS info (device dm-8): checking UUID tree
[   91.668256] BTRFS info (device dm-8): unmounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2

Link: https://github.com/kdave/btrfs-progs/issues/689
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 1 +
 fs/btrfs/super.c   | 6 +++++-
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 350e1b02cc8e..2fef94bfa2ff 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3209,6 +3209,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 		goto fail_alloc;
 	}
 
+	btrfs_info(fs_info, "mounting filesystem %pU", disk_super->fsid);
 	/*
 	 * Verify the type first, if that or the checksum value are
 	 * corrupted, we'll find out
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 6ecf78d09694..fbcd8c8d23dc 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -80,7 +80,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data);
 
 static void btrfs_put_super(struct super_block *sb)
 {
-	close_ctree(btrfs_sb(sb));
+	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
+
+	btrfs_info(fs_info, "unmounting filesystem %pU",
+		   fs_info->fs_devices->fsid);
+	close_ctree(fs_info);
 }
 
 enum {
-- 
2.42.0

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Anand Jain]

On 11/2/23 05:24, Qu Wenruo wrote:
> There is a feature request to add dmesg output when unmounting a btrfs.
> 
> There are several alternative methods to do the same thing, but with
> their problems:
> 
> - Use eBPF to watch btrfs_put_super()/open_ctree()
>    Not end user friendly, they have to dip their head into the source
>    code.
> 
> - Watch for /sys/fs/<uuid>/
>    This is way more simpler, but still requires some simple device -> uuid
>    lookups.
>    And a script needs to use inotify to watch /sys/fs/.
> 


> Compared to all these, directly outputting the information into dmesg
> would be the most simple one, with both device and UUID included.
> 

Well, I submitted a patch for this in 2017, but I'm not sure why it 
wasn't integrated or commented.

https://lore.kernel.org/linux-btrfs/3352043d-dbb1-0055-f50a-c91ca43aff1d@oracle.com/

Thanks, Anand


> And since we're here, also add the output when mounting a btrfs, to keep
> the dmesg paired.
> 
> Now mounting a btrfs with all default mkfs options would look like this:
> 
> [   81.906566] BTRFS info (device dm-8): mounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> [   81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
> [   81.908258] BTRFS info (device dm-8): using free space tree
> [   81.912644] BTRFS info (device dm-8): auto enabling async discard
> [   81.913277] BTRFS info (device dm-8): checking UUID tree
> [   91.668256] BTRFS info (device dm-8): unmounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> 
> Link: https://github.com/kdave/btrfs-progs/issues/689
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/disk-io.c | 1 +
>   fs/btrfs/super.c   | 6 +++++-
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 350e1b02cc8e..2fef94bfa2ff 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3209,6 +3209,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>   		goto fail_alloc;
>   	}
>   
> +	btrfs_info(fs_info, "mounting filesystem %pU", disk_super->fsid);
>   	/*
>   	 * Verify the type first, if that or the checksum value are
>   	 * corrupted, we'll find out
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 6ecf78d09694..fbcd8c8d23dc 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -80,7 +80,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data);
>   
>   static void btrfs_put_super(struct super_block *sb)
>   {
> -	close_ctree(btrfs_sb(sb));
> +	struct btrfs_fs_info *fs_info = btrfs_sb(sb);
> +
> +	btrfs_info(fs_info, "unmounting filesystem %pU",
> +		   fs_info->fs_devices->fsid);
> +	close_ctree(fs_info);
>   }
>   
>   enum {

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[David Sterba]

On Thu, Nov 02, 2023 at 09:16:19AM +0800, Anand Jain wrote:
> On 11/2/23 05:24, Qu Wenruo wrote:
> > There is a feature request to add dmesg output when unmounting a btrfs.
> > 
> > There are several alternative methods to do the same thing, but with
> > their problems:
> > 
> > - Use eBPF to watch btrfs_put_super()/open_ctree()
> >    Not end user friendly, they have to dip their head into the source
> >    code.
> > 
> > - Watch for /sys/fs/<uuid>/
> >    This is way more simpler, but still requires some simple device -> uuid
> >    lookups.
> >    And a script needs to use inotify to watch /sys/fs/.
> > 
> 
> 
> > Compared to all these, directly outputting the information into dmesg
> > would be the most simple one, with both device and UUID included.
> > 
> 
> Well, I submitted a patch for this in 2017, but I'm not sure why it 
> wasn't integrated or commented.
> 
> https://lore.kernel.org/linux-btrfs/3352043d-dbb1-0055-f50a-c91ca43aff1d@oracle.com/

I think you sent it more than once and that I replied, messages about
mounting each subvolume (as in your patch) seem to be duplicating what
eg. audit or some policy layer should do. The same comment is in the
github issue.

The patch here and the request is to track only the first and last mount
of the same filesystem with a bit different use case, eg. properly
waiting until lazy umount finishe, or in container environments to be
sure that the last mount is gone.

The suggested waiting on /sys/fs/btrfs/UUID could eventually wrapped in
a subcommand, for conveniencie.

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Qu Wenruo]

On 2023/11/3 06:48, David Sterba wrote:
> On Thu, Nov 02, 2023 at 09:16:19AM +0800, Anand Jain wrote:
>> On 11/2/23 05:24, Qu Wenruo wrote:
>>> There is a feature request to add dmesg output when unmounting a btrfs.
>>>
>>> There are several alternative methods to do the same thing, but with
>>> their problems:
>>>
>>> - Use eBPF to watch btrfs_put_super()/open_ctree()
>>>     Not end user friendly, they have to dip their head into the source
>>>     code.
>>>
>>> - Watch for /sys/fs/<uuid>/
>>>     This is way more simpler, but still requires some simple device -> uuid
>>>     lookups.
>>>     And a script needs to use inotify to watch /sys/fs/.
>>>
>>
>>
>>> Compared to all these, directly outputting the information into dmesg
>>> would be the most simple one, with both device and UUID included.
>>>
>>
>> Well, I submitted a patch for this in 2017, but I'm not sure why it
>> wasn't integrated or commented.
>>
>> https://lore.kernel.org/linux-btrfs/3352043d-dbb1-0055-f50a-c91ca43aff1d@oracle.com/
>
> I think you sent it more than once and that I replied, messages about
> mounting each subvolume (as in your patch) seem to be duplicating what
> eg. audit or some policy layer should do. The same comment is in the
> github issue.
>
> The patch here and the request is to track only the first and last mount
> of the same filesystem with a bit different use case, eg. properly
> waiting until lazy umount finishe, or in container environments to be
> sure that the last mount is gone.
>
> The suggested waiting on /sys/fs/btrfs/UUID could eventually wrapped in
> a subcommand, for conveniencie.

My bad, I didn't realize until I crafted the `btrfs monitor watch` command.

Inotify just doesn't work on sysfs....

So I believe for now, the patch is the best way to go, unless we want to
something like checking the sysfs every second...

Thanks,
Qu

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[David Sterba]

On Thu, Nov 02, 2023 at 07:54:50AM +1030, Qu Wenruo wrote:
> There is a feature request to add dmesg output when unmounting a btrfs.
> 
> There are several alternative methods to do the same thing, but with
> their problems:
> 
> - Use eBPF to watch btrfs_put_super()/open_ctree()
>   Not end user friendly, they have to dip their head into the source
>   code.
> 
> - Watch for /sys/fs/<uuid>/
>   This is way more simpler, but still requires some simple device -> uuid
>   lookups.
>   And a script needs to use inotify to watch /sys/fs/.
> 
> Compared to all these, directly outputting the information into dmesg
> would be the most simple one, with both device and UUID included.
> 
> And since we're here, also add the output when mounting a btrfs, to keep
> the dmesg paired.
> 
> Now mounting a btrfs with all default mkfs options would look like this:
> 
> [   81.906566] BTRFS info (device dm-8): mounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> [   81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
> [   81.908258] BTRFS info (device dm-8): using free space tree
> [   81.912644] BTRFS info (device dm-8): auto enabling async discard
> [   81.913277] BTRFS info (device dm-8): checking UUID tree
> [   91.668256] BTRFS info (device dm-8): unmounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2

On a fresh look, I' suggest to write the messages like

BTRFS info (...): first mount of filesystem UUID
...
BTRFS info (...): last umount of filesystem UUID

This is not for each mount so it's slightly confusing.

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Qu Wenruo]

On 2023/11/14 04:15, David Sterba wrote:
> On Thu, Nov 02, 2023 at 07:54:50AM +1030, Qu Wenruo wrote:
>> There is a feature request to add dmesg output when unmounting a btrfs.
>>
>> There are several alternative methods to do the same thing, but with
>> their problems:
>>
>> - Use eBPF to watch btrfs_put_super()/open_ctree()
>>    Not end user friendly, they have to dip their head into the source
>>    code.
>>
>> - Watch for /sys/fs/<uuid>/
>>    This is way more simpler, but still requires some simple device -> uuid
>>    lookups.
>>    And a script needs to use inotify to watch /sys/fs/.
>>
>> Compared to all these, directly outputting the information into dmesg
>> would be the most simple one, with both device and UUID included.
>>
>> And since we're here, also add the output when mounting a btrfs, to keep
>> the dmesg paired.
>>
>> Now mounting a btrfs with all default mkfs options would look like this:
>>
>> [   81.906566] BTRFS info (device dm-8): mounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
>> [   81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
>> [   81.908258] BTRFS info (device dm-8): using free space tree
>> [   81.912644] BTRFS info (device dm-8): auto enabling async discard
>> [   81.913277] BTRFS info (device dm-8): checking UUID tree
>> [   91.668256] BTRFS info (device dm-8): unmounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
>
> On a fresh look, I' suggest to write the messages like
>
> BTRFS info (...): first mount of filesystem UUID
> ...
> BTRFS info (...): last umount of filesystem UUID
>
> This is not for each mount so it's slightly confusing.
>
Sure, I'm totally fine to change the words.

Do I need to resend or the change is small enough to get updated in the
branch?

Thanks,
Qu

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[David Sterba]

On Tue, Nov 14, 2023 at 06:37:15AM +1030, Qu Wenruo wrote:
> 
> 
> On 2023/11/14 04:15, David Sterba wrote:
> > On Thu, Nov 02, 2023 at 07:54:50AM +1030, Qu Wenruo wrote:
> >> There is a feature request to add dmesg output when unmounting a btrfs.
> >>
> >> There are several alternative methods to do the same thing, but with
> >> their problems:
> >>
> >> - Use eBPF to watch btrfs_put_super()/open_ctree()
> >>    Not end user friendly, they have to dip their head into the source
> >>    code.
> >>
> >> - Watch for /sys/fs/<uuid>/
> >>    This is way more simpler, but still requires some simple device -> uuid
> >>    lookups.
> >>    And a script needs to use inotify to watch /sys/fs/.
> >>
> >> Compared to all these, directly outputting the information into dmesg
> >> would be the most simple one, with both device and UUID included.
> >>
> >> And since we're here, also add the output when mounting a btrfs, to keep
> >> the dmesg paired.
> >>
> >> Now mounting a btrfs with all default mkfs options would look like this:
> >>
> >> [   81.906566] BTRFS info (device dm-8): mounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> >> [   81.907494] BTRFS info (device dm-8): using crc32c (crc32c-intel) checksum algorithm
> >> [   81.908258] BTRFS info (device dm-8): using free space tree
> >> [   81.912644] BTRFS info (device dm-8): auto enabling async discard
> >> [   81.913277] BTRFS info (device dm-8): checking UUID tree
> >> [   91.668256] BTRFS info (device dm-8): unmounting filesystem 633b5c16-afe3-4b79-b195-138fe145e4f2
> >
> > On a fresh look, I' suggest to write the messages like
> >
> > BTRFS info (...): first mount of filesystem UUID
> > ...
> > BTRFS info (...): last umount of filesystem UUID
> >
> > This is not for each mount so it's slightly confusing.
> >
> Sure, I'm totally fine to change the words.
> 
> Do I need to resend or the change is small enough to get updated in the
> branch?

No need to resend for such changes, is more for the feedback and sanity
check.

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Anand Jain]

looks good.

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Anand Jain]

On misc-next:

     3212 btrfs_info(fs_info, "frist mount of filesystem %pU", 
disk_super->fsid);

Has typo error.



On 11/14/23 08:38, Anand Jain wrote:
> 
> looks good.
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[David Sterba]

On Tue, Nov 14, 2023 at 07:12:43PM +0800, Anand Jain wrote:
> 
> On misc-next:
> 
>      3212 btrfs_info(fs_info, "frist mount of filesystem %pU", 
> disk_super->fsid);
> 
> Has typo error.

Fixed, thanks for catching it.

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Anand Jain]

Also, is it possible to CC the stable kernel? It's not a bug fix but a 
debug essential patch.

Thx, Anand

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[Anand Jain]

Starting from v5.4?

On 11/21/23 13:18, Anand Jain wrote:
> Also, is it possible to CC the stable kernel? It's not a bug fix but a 
> debug essential patch.
> 
> Thx, Anand
> 

Re: [PATCH] btrfs: add dmesg output when mounting and unmounting

[David Sterba]

On Tue, Nov 21, 2023 at 01:18:23PM +0800, Anand Jain wrote:
> Also, is it possible to CC the stable kernel? It's not a bug fix but a 
> debug essential patch.

Yes we can do that, it's a usability improvement.