| Message ID | 215e7eea95459d1b0cc4fd9ce522dc7c8f5d4e02.1698873846.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: add dmesg output when mounting and unmounting | expand |
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 {
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.
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
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.
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
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.
looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
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>
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.
Also, is it possible to CC the stable kernel? It's not a bug fix but a debug essential patch. Thx, Anand
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 >
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.
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 {
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(-)