When attempting to exclusive open a device which has no exclusive open permission, such as a physical device associated with the flakey dm device, the open operation will fail, resulting in a mount failure. In this particular scenario, we erroneously return -EINVAL instead of the correct error code provided by the bdev_open_by_path() function, which is -EBUSY. Fix this, by returning error code from the bdev_open_by_path() function. With this correction, the mount error message will align with that of ext4 and xfs. Reviewed-by: Boris Burkov <boris@bur.io> Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v2: rename ret_err in v1 to ret and existing ret to ret2. v1: https://lore.kernel.org/all/dfe752bda3e3d57c352725a4951e332b016506a9.1709991269.git.anand.jain@oracle.com/ fs/btrfs/volumes.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 5f002347d167..7919df386332 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1184,23 +1184,30 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, struct btrfs_device *device; struct btrfs_device *latest_dev = NULL; struct btrfs_device *tmp_device; + int ret = 0; list_for_each_entry_safe(device, tmp_device, &fs_devices->devices, dev_list) { - int ret; + int ret2; - ret = btrfs_open_one_device(fs_devices, device, flags, holder); - if (ret == 0 && + ret2 = btrfs_open_one_device(fs_devices, device, flags, holder); + if (ret2 == 0 && (!latest_dev || device->generation > latest_dev->generation)) { latest_dev = device; - } else if (ret == -ENODATA) { + } else if (ret2 == -ENODATA) { fs_devices->num_devices--; list_del(&device->dev_list); btrfs_free_device(device); } + if (ret == 0 && ret2 != 0) + ret = ret2; } - if (fs_devices->open_devices == 0) + + if (fs_devices->open_devices == 0) { + if (ret) + return ret; return -EINVAL; + } fs_devices->opened = 1; fs_devices->latest_dev = latest_dev; -- 2.38.1
On 3/19/24 04:06, David Sterba wrote:
> On Sat, Mar 09, 2024 at 07:16:35PM +0530, Anand Jain wrote:
>> When attempting to exclusive open a device which has no exclusive open
>> permission, such as a physical device associated with the flakey dm
>> device, the open operation will fail, resulting in a mount failure.
>>
>> In this particular scenario, we erroneously return -EINVAL instead of the
>> correct error code provided by the bdev_open_by_path() function, which is
>> -EBUSY.
>>
>> Fix this, by returning error code from the bdev_open_by_path() function.
>> With this correction, the mount error message will align with that of
>> ext4 and xfs.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>> fs/btrfs/volumes.c | 9 ++++++++-
>> 1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bb0857cfbef2..8a35605822bf 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>> struct btrfs_device *device;
>> struct btrfs_device *latest_dev = NULL;
>> struct btrfs_device *tmp_device;
>> + int ret_err = 0;
>
> Please use 'ret' here
>
>>
>> list_for_each_entry_safe(device, tmp_device, &fs_devices->devices,
>> dev_list) {
>> @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices,
>> list_del(&device->dev_list);
>> btrfs_free_device(device);
>> }
>> + if (ret_err == 0 && ret != 0)
>
> and rename the original 'ret' used in the scope as 'ret2', this is the
> preferred pattern. For simple changes within one function it's ok to do
> it in one patch.
Yep. Done.
Thanks, Anand
在 2024/3/19 11:03, kind.moon1862@fastmail.com 写道:
> On Mon, Mar 18, 2024, at 17:06, Qu Wenruo wrote:
>
>> Use "mount -o rescue=all,ro" instead.
>
> Thank you for your suggestion. Should I run it under CentOS 7 (the original OS) or under the newer kernel in the SystemRescue distro?
Newer kernel please.
And still, please run memtest before doing any rescue.
As faulty hardware can always lead to weird problems no matter what.
And CentOS 7 is not recommended for btrfs usage (no proper btrfs related
backport and old kernels, IIRC it's already EOL)
Thanks,
Qu
On Mon, Mar 18, 2024, at 17:06, Qu Wenruo wrote:
> Use "mount -o rescue=all,ro" instead.
Thank you for your suggestion. Should I run it under CentOS 7 (the original OS) or under the newer kernel in the SystemRescue distro?
在 2024/3/19 08:47, David Sterba 写道:
> On Thu, Mar 14, 2024 at 04:19:00PM -0700, Boris Burkov wrote:
>> not sure exactly how this ifdef was supposed to work originally, but it
>> currently doesn't and I don't see other use cases of this pattern.
>>
>> Use EXPERIMENTAL which does work after:
>
> Ok, makes sense. I missed that because I searched for EXPERIMENTAL and
> did not find anything relevant but CONFIG_BTRFS_DEBUG has an effect.
> I'll add a comment so it does not get accidentally changed back when
> syncin the kernel code. Thanks.
>
Can't we just convert CONFIG_BTRFS_DEBUG to EXPERIMENTAL in kerncompat.h?
Thanks,
Qu
在 2024/3/19 07:52, kind.moon1862@fastmail.com 写道: > > Dear btrfs team, > > I've been asked to help someone with a btrfs problem. They use CentOS 7. I've been told the host was shut down cleanly, but it won't mount a btrfs filesystem when powering on. > > Below is what I've done so far. Any guidance would be appreciated! > > I booted using SystemRescue 11.0. Some info about the rescue OS: > > # uname -a > Linux sysrescue 6.6.14-1-lts #1 SMP PREEMPT_DYNAMIC Fri, 26 Jan 2024 11:54:58 +0000 x86_64 GNU/Linux > > # btrfs --version > btrfs-progs v6.6.3 > > Here are the disks: > > # btrfs fi show > Label: '<redacted>' uuid: 834b4bfd-fbd4-40b9-8868-d1187c3c5a63 > Total devices 4 FS bytes used 17.44TiB > devid 1 size 9.10TiB used 8.94TiB path /dev/sda > devid 2 size 9.10TiB used 8.94TiB path /dev/sdb > devid 3 size 9.10TiB used 8.94TiB path /dev/sdc > devid 4 size 9.10TiB used 8.94TiB path /dev/sdd > > I tried mounting: > > # mount -t btrfs -o recovery,ro /dev/sda /mnt/data > mount: /mnt/data: can't read superblock on /dev/sda. > dmesg(1) may have more information after failed mount system call. Use "mount -o rescue=all,ro" instead. > > dmesg shows this: > > [ 1940.962632] BTRFS info (device sda): first mount of filesystem 834b4bfd-fbd4-40b9-8868-d1187c3c5a63 > [ 1940.962665] BTRFS info (device sda): using crc32c (crc32c-intel) checksum algorithm > [ 1940.962686] BTRFS warning (device sda): 'recovery' is deprecated, use 'rescue=usebackuproot' instead > [ 1940.962692] BTRFS info (device sda): trying to use backup root at mount time > [ 1940.962698] BTRFS info (device sda): disk space caching is enabled > [ 1942.115796] BTRFS critical (device sda): corrupt leaf: root=2 block=11438351450112 slot=170, invalid key objectid, have 5822947745796 expect to be aligned to 4096 Bad extent item start bytenr, 5822947745796 = 0x54bc2bb6004, the last 0x004 looks like a bitflip. Please run memtest to make sure your hardware memory is fine, or such problem would just happen again and again. Thankfully extent tree corruption is not a big deal for "rescue=all,ro" mount. Thanks, Qu > [ 1942.115819] BTRFS error (device sda): read time tree block corruption detected on logical 11438351450112 mirror 2 > [ 1942.116170] BTRFS critical (device sda): corrupt leaf: root=2 block=11438351450112 slot=170, invalid key objectid, have 5822947745796 expect to be aligned to 4096 > [ 1942.116183] BTRFS error (device sda): read time tree block corruption detected on logical 11438351450112 mirror 1 > [ 1942.116212] BTRFS error (device sda): failed to read block groups: -5 > [ 1942.140369] BTRFS error (device sda): open_ctree failed > > I tried to recover the superblock: > > # btrfs rescue super-recover -v /dev/sda > > All Devices: > Device: id = 3, name = /dev/sdc > Device: id = 2, name = /dev/sdb > Device: id = 4, name = /dev/sdd > Device: id = 1, name = /dev/sda > > Before Recovering: > [All good supers]: > device name = /dev/sdc > superblock bytenr = 65536 > > device name = /dev/sdc > superblock bytenr = 67108864 > > device name = /dev/sdc > superblock bytenr = 274877906944 > > device name = /dev/sdb > superblock bytenr = 65536 > > device name = /dev/sdb > superblock bytenr = 67108864 > > device name = /dev/sdb > superblock bytenr = 274877906944 > > device name = /dev/sdd > superblock bytenr = 65536 > > device name = /dev/sdd > superblock bytenr = 67108864 > > device name = /dev/sdd > superblock bytenr = 274877906944 > > device name = /dev/sda > superblock bytenr = 65536 > > device name = /dev/sda > superblock bytenr = 67108864 > > device name = /dev/sda > superblock bytenr = 274877906944 > > [All bad supers]: > > All supers are valid, no need to recover > > I checked the filesystem: > > # btrfs check --readonly /dev/sda > Opening filesystem to check... > corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) > corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) > corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) > corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) > ERROR: failed to read block groups: Operation not permitted > ERROR: cannot open file system > >
On Sat, Mar 09, 2024 at 07:16:35PM +0530, Anand Jain wrote: > When attempting to exclusive open a device which has no exclusive open > permission, such as a physical device associated with the flakey dm > device, the open operation will fail, resulting in a mount failure. > > In this particular scenario, we erroneously return -EINVAL instead of the > correct error code provided by the bdev_open_by_path() function, which is > -EBUSY. > > Fix this, by returning error code from the bdev_open_by_path() function. > With this correction, the mount error message will align with that of > ext4 and xfs. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/volumes.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index bb0857cfbef2..8a35605822bf 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, > struct btrfs_device *device; > struct btrfs_device *latest_dev = NULL; > struct btrfs_device *tmp_device; > + int ret_err = 0; Please use 'ret' here > > list_for_each_entry_safe(device, tmp_device, &fs_devices->devices, > dev_list) { > @@ -1205,9 +1206,15 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, > list_del(&device->dev_list); > btrfs_free_device(device); > } > + if (ret_err == 0 && ret != 0) and rename the original 'ret' used in the scope as 'ret2', this is the preferred pattern. For simple changes within one function it's ok to do it in one patch.
On Thu, Mar 14, 2024 at 09:50:31AM -0700, Boris Burkov wrote: > On Thu, Mar 14, 2024 at 07:09:24PM +0530, Anand Jain wrote: > > > > And this one as well, for the review. Thx. > > > > > > On 3/9/24 19:16, Anand Jain wrote: > > > When attempting to exclusive open a device which has no exclusive open > > > permission, such as a physical device associated with the flakey dm > > > device, the open operation will fail, resulting in a mount failure. > > > > > > In this particular scenario, we erroneously return -EINVAL instead of the > > > correct error code provided by the bdev_open_by_path() function, which is > > > -EBUSY. > > > > > > Fix this, by returning error code from the bdev_open_by_path() function. > > > With this correction, the mount error message will align with that of > > > ext4 and xfs. > > > > > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > One small "nit", but LGTM. > > Reviewed-by: Boris Burkov <boris@bur.io> > > > > --- > > > fs/btrfs/volumes.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > > > index bb0857cfbef2..8a35605822bf 100644 > > > --- a/fs/btrfs/volumes.c > > > +++ b/fs/btrfs/volumes.c > > > @@ -1191,6 +1191,7 @@ static int open_fs_devices(struct btrfs_fs_devices *fs_devices, > > > struct btrfs_device *device; > > > struct btrfs_device *latest_dev = NULL; > > > struct btrfs_device *tmp_device; > > > + int ret_err = 0; > > A quick grep shows that "err" is a much more common name for the > variable when using this pattern in btrfs. Well 'err' is there for historical reasons and the pattern we'd like to use everywhere is to have 'ret' matching the function return type or a common variable for any random function called. If there's need for more than one 'ret' (so the function-wide is not overwritten) it's ret2 etc. https://btrfs.readthedocs.io/en/latest/dev/Development-notes.html#code Patches to convert err -> ret are also welcome as it can be confusing what to use in new code when there are two ways.
On Thu, Mar 14, 2024 at 04:19:00PM -0700, Boris Burkov wrote:
> not sure exactly how this ifdef was supposed to work originally, but it
> currently doesn't and I don't see other use cases of this pattern.
>
> Use EXPERIMENTAL which does work after:
Ok, makes sense. I missed that because I searched for EXPERIMENTAL and
did not find anything relevant but CONFIG_BTRFS_DEBUG has an effect.
I'll add a comment so it does not get accidentally changed back when
syncin the kernel code. Thanks.
On Mon, Mar 18, 2024 at 11:11:56AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When unlocking a write lock on a drew lock, at btrfs_drew_write_unlock(),
> it's pointless to wake up tasks waiting to acquire a read lock if we
> didn't decrement the 'writers' counter down to 0, since a read lock can
> only be acquired when the counter reaches a value of 0. Doing so is
> harmless from a functional point of view, but it's not efficient due to
> unnecessarily waking up tasks just for them to sleep again on the
> waitqueue.
>
> So change this to wake up readers only if we decremented the 'writers'
> counter to 0.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
On Mon, Mar 18, 2024 at 11:02:19PM +0100, David Sterba wrote:
> What's the purpose of shared/ ? We have tests that make sense for a
> subset of supported filesystems in generic/, with proper _required and
> other the checks it works fine.
Yes. I'd rather get rid of shared and the few tets in there as it just
creats confusion.
On Sat, Mar 16, 2024 at 10:32:33PM +0530, Anand Jain wrote:
> Given that ext4 also allows mounting of a cloned filesystem, the btrfs
> test case btrfs/312, which assesses the functionality of cloned filesystem
> support, can be refactored to be under the shared group.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v2:
> Move to shared testcase instead of generic.
What's the purpose of shared/ ? We have tests that make sense for a
subset of supported filesystems in generic/, with proper _required and
other the checks it works fine.
I see that v1 did the move to generic/ but then the 'shared' got
suggested, which is IMHO the wrong direction. I remember some distant
past discussions about shared/ and what to put there. Right now there
are 3 remaining tests which I think is a good opportunity to make it 0.
On Tue, Mar 19, 2024 at 06:42:26AM +1030, Qu Wenruo wrote:
>
>
> 在 2024/3/18 22:44, fdmanana@kernel.org 写道:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Trivial stuff, details in the change logs.
>
> I guess it's just exposed by some random code reading?
>
> No automatic tools to expose such single line wrapper?
---
dentifier FUNC, CALL;
type TYPE;
@@
TYPE
FUNC(...)
{
* return CALL(...);
}
---
but not all trivial helpers should be removed, some of them have a semantic
value or pair another fuction that is not trivial.
Dear btrfs team, I've been asked to help someone with a btrfs problem. They use CentOS 7. I've been told the host was shut down cleanly, but it won't mount a btrfs filesystem when powering on. Below is what I've done so far. Any guidance would be appreciated! I booted using SystemRescue 11.0. Some info about the rescue OS: # uname -a Linux sysrescue 6.6.14-1-lts #1 SMP PREEMPT_DYNAMIC Fri, 26 Jan 2024 11:54:58 +0000 x86_64 GNU/Linux # btrfs --version btrfs-progs v6.6.3 Here are the disks: # btrfs fi show Label: '<redacted>' uuid: 834b4bfd-fbd4-40b9-8868-d1187c3c5a63 Total devices 4 FS bytes used 17.44TiB devid 1 size 9.10TiB used 8.94TiB path /dev/sda devid 2 size 9.10TiB used 8.94TiB path /dev/sdb devid 3 size 9.10TiB used 8.94TiB path /dev/sdc devid 4 size 9.10TiB used 8.94TiB path /dev/sdd I tried mounting: # mount -t btrfs -o recovery,ro /dev/sda /mnt/data mount: /mnt/data: can't read superblock on /dev/sda. dmesg(1) may have more information after failed mount system call. dmesg shows this: [ 1940.962632] BTRFS info (device sda): first mount of filesystem 834b4bfd-fbd4-40b9-8868-d1187c3c5a63 [ 1940.962665] BTRFS info (device sda): using crc32c (crc32c-intel) checksum algorithm [ 1940.962686] BTRFS warning (device sda): 'recovery' is deprecated, use 'rescue=usebackuproot' instead [ 1940.962692] BTRFS info (device sda): trying to use backup root at mount time [ 1940.962698] BTRFS info (device sda): disk space caching is enabled [ 1942.115796] BTRFS critical (device sda): corrupt leaf: root=2 block=11438351450112 slot=170, invalid key objectid, have 5822947745796 expect to be aligned to 4096 [ 1942.115819] BTRFS error (device sda): read time tree block corruption detected on logical 11438351450112 mirror 2 [ 1942.116170] BTRFS critical (device sda): corrupt leaf: root=2 block=11438351450112 slot=170, invalid key objectid, have 5822947745796 expect to be aligned to 4096 [ 1942.116183] BTRFS error (device sda): read time tree block corruption detected on logical 11438351450112 mirror 1 [ 1942.116212] BTRFS error (device sda): failed to read block groups: -5 [ 1942.140369] BTRFS error (device sda): open_ctree failed I tried to recover the superblock: # btrfs rescue super-recover -v /dev/sda All Devices: Device: id = 3, name = /dev/sdc Device: id = 2, name = /dev/sdb Device: id = 4, name = /dev/sdd Device: id = 1, name = /dev/sda Before Recovering: [All good supers]: device name = /dev/sdc superblock bytenr = 65536 device name = /dev/sdc superblock bytenr = 67108864 device name = /dev/sdc superblock bytenr = 274877906944 device name = /dev/sdb superblock bytenr = 65536 device name = /dev/sdb superblock bytenr = 67108864 device name = /dev/sdb superblock bytenr = 274877906944 device name = /dev/sdd superblock bytenr = 65536 device name = /dev/sdd superblock bytenr = 67108864 device name = /dev/sdd superblock bytenr = 274877906944 device name = /dev/sda superblock bytenr = 65536 device name = /dev/sda superblock bytenr = 67108864 device name = /dev/sda superblock bytenr = 274877906944 [All bad supers]: All supers are valid, no need to recover I checked the filesystem: # btrfs check --readonly /dev/sda Opening filesystem to check... corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) corrupt leaf: root=2 block=62421692628992 slot=7, bad key order, prev (7275590123520 168 11227136) current (7258421481472 168 10948608) ERROR: failed to read block groups: Operation not permitted ERROR: cannot open file system
Hello: This series was applied to jaegeuk/f2fs.git (dev) by Jens Axboe <axboe@kernel.dk>: On Sun, 28 Jan 2024 23:52:15 -0800 you wrote: > Fueled by the LSFMM discussion on removing GFP_NOFS initiated by Willy, > I've looked into the sole GFP_NOFS allocation in zonefs. As it turned out, > it is only done for zone management commands and can be removed. > > After digging into more callers of blkdev_zone_mgmt() I came to the > conclusion that the gfp_mask parameter can be removed alltogether. > > [...] Here is the summary with links: - [f2fs-dev,v3,1/5] zonefs: pass GFP_KERNEL to blkdev_zone_mgmt() call https://git.kernel.org/jaegeuk/f2fs/c/9105ce591b42 - [f2fs-dev,v3,2/5] dm: dm-zoned: guard blkdev_zone_mgmt with noio scope https://git.kernel.org/jaegeuk/f2fs/c/218082010ace - [f2fs-dev,v3,3/5] btrfs: zoned: call blkdev_zone_mgmt in nofs scope https://git.kernel.org/jaegeuk/f2fs/c/d9d556755f16 - [f2fs-dev,v3,4/5] f2fs: guard blkdev_zone_mgmt with nofs scope https://git.kernel.org/jaegeuk/f2fs/c/147ec1c60e32 - [f2fs-dev,v3,5/5] block: remove gfp_flags from blkdev_zone_mgmt https://git.kernel.org/jaegeuk/f2fs/c/71f4ecdbb42a You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
On Mon, Mar 18, 2024 at 8:12 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/3/18 22:44, fdmanana@kernel.org 写道: > > From: Filipe Manana <fdmanana@suse.com> > > > > Trivial stuff, details in the change logs. > > I guess it's just exposed by some random code reading? Yes, by reading. > > No automatic tools to expose such single line wrapper? Not that I know of. Maybe some coccinelle script could do it, dunno. > > > > > Filipe Manana (2): > > btrfs: remove pointless readahead callback wrapper > > btrfs: remove pointless writepages callback wrapper > > Anyway looks good to me. > > Reviewed-by: Qu Wenruo <wqu@suse.com> > > Thanks, > Qu > > > > fs/btrfs/extent_io.c | 5 ++--- > > fs/btrfs/extent_io.h | 5 ++--- > > fs/btrfs/inode.c | 11 ----------- > > 3 files changed, 4 insertions(+), 17 deletions(-) > >
在 2024/3/18 22:44, fdmanana@kernel.org 写道: > From: Filipe Manana <fdmanana@suse.com> > > Trivial stuff, details in the change logs. I guess it's just exposed by some random code reading? No automatic tools to expose such single line wrapper? > > Filipe Manana (2): > btrfs: remove pointless readahead callback wrapper > btrfs: remove pointless writepages callback wrapper Anyway looks good to me. Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > > fs/btrfs/extent_io.c | 5 ++--- > fs/btrfs/extent_io.h | 5 ++--- > fs/btrfs/inode.c | 11 ----------- > 3 files changed, 4 insertions(+), 17 deletions(-) >
在 2024/3/19 00:26, Tavian Barnes 写道: > We recently tracked down a race condition that triggered a read for an > extent buffer with EXTENT_BUFFER_UPTODATE already set. While this read > was in progress, other concurrent readers would see the UPTODATE bit and > return early as if the read was already complete, making accesses to the > extent buffer conflict with the read operation that was overwriting it. > > Add a WARN_ON() to end_bbio_meta_read() for this situation to make > similar races easier to spot in the future. > > Signed-off-by: Tavian Barnes <tavianator@tavianator.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/extent_io.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 46173dcfde4f..0024ea20bfc4 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4285,6 +4285,13 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio) > struct folio_iter fi; > u32 bio_offset = 0; > > + /* > + * If the extent buffer is marked UPTODATE before the read operation > + * completes, other calls to read_extent_buffer_pages() will return > + * early without waiting for the read to finish, causing data races. > + */ > + WARN_ON(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags)); > + > eb->read_mirror = bbio->mirror_num; > > if (uptodate &&
在 2024/3/19 00:26, Tavian Barnes 写道: > We are clearing the bit and waking up any waiters in two different > places. Factor that code out into a static helper function. > > Signed-off-by: Tavian Barnes <tavianator@tavianator.com> Reviewed-by: Qu Wenruo <wqu@suse.com> Thanks, Qu > --- > fs/btrfs/extent_io.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 61594eaf1f89..46173dcfde4f 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -4270,6 +4270,13 @@ void set_extent_buffer_uptodate(struct extent_buffer *eb) > } > } > > +static void clear_extent_buffer_reading(struct extent_buffer *eb) > +{ > + clear_bit(EXTENT_BUFFER_READING, &eb->bflags); > + smp_mb__after_atomic(); > + wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING); > +} > + > static void end_bbio_meta_read(struct btrfs_bio *bbio) > { > struct extent_buffer *eb = bbio->private; > @@ -4304,9 +4311,7 @@ static void end_bbio_meta_read(struct btrfs_bio *bbio) > bio_offset += len; > } > > - clear_bit(EXTENT_BUFFER_READING, &eb->bflags); > - smp_mb__after_atomic(); > - wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING); > + clear_extent_buffer_reading(eb); > free_extent_buffer(eb); > > bio_put(&bbio->bio); > @@ -4340,9 +4345,7 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num, > * will now be set, and we shouldn't read it in again. > */ > if (unlikely(test_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags))) { > - clear_bit(EXTENT_BUFFER_READING, &eb->bflags); > - smp_mb__after_atomic(); > - wake_up_bit(&eb->bflags, EXTENT_BUFFER_READING); > + clear_extent_buffer_reading(eb); > return 0; > } >
在 2024/3/19 02:56, Filipe Manana 写道: > On Thu, Mar 14, 2024 at 8:56 PM Qu Wenruo <wqu@suse.com> wrote: >> >> >> >> 在 2024/3/15 04:10, Filipe Manana 写道: >>> On Thu, Mar 14, 2024 at 9:51 AM Qu Wenruo <wqu@suse.com> wrote: >>>> >>>> Currently the scrub error report is pretty long: >>>> >>>> BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872 on dev /dev/mapper/test-scratch1 physical 13647872 >>>> BTRFS warning (device dm-2): checksum error at logical 13647872 on dev /dev/mapper/test-scratch1, physical 13647872, root 5, inode 257, offset 16384, length 4096, links 1 (path: file1) >>>> >>>> Since we have so many things to output, it's not a surprise we got long >>>> error lines. >>>> >>>> To make the lines a little shorter, and make important info more >>>> obvious, change the unify output to something like this: >>>> >>>> BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872 >>>> BTRFS warning (device dm-2): checksum error at inode 5/257(file1) fileoff 16384, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872 >>> >>> I find that hard to read because: >>> >>> 1) Please leave spaces before opening parenthesis. >>> That makes things less cluttered and easier to the eyes, more >>> consistent with what we generally do and it's the formal way to do >>> things in English (see >>> https://www.scribens.com/grammar-rules/typography/spacing.html); >> >> Yep, I can do that, but I also want to keep the involved members >> together thus closer. >> >> Not sure if adding spaces would still keep the close relationships >> between the values. >> >> E.g: inode 5/256 (file1) fileoff 16384, logical 123456 (1) physical 1 >> (scratch1) 123456 >> >> It makes it a little harder to indicate that "(1)" is related to logical >> address (thus mirror number). >> >>> >>> 2) Instead of "inode 5/257(file1)", something a lot easier to >>> understand like "inode 5 root 257 path \"file1\"", which leaves no >>> margin for doubt about what each number is; >>> >>> 3) Same with "logical 13647872(1)" - what is the 1? That will be the >>> question anyone reading such a log message will likely have. >>> Something like "logical 13647872 mirror 1" makes it clear; >>> >>> 4) Same with "physical 1(/dev/mapper/test-scratch1)13647872". >>> Something like "physical 13647872 device ID 1 device path >>> \"/dev/mapper/test-scratch1\"", makes it clear what each number is and >>> easier to read. >> >> I totally understand the original output format is much easier to read >> on its own. >> >> However if we have hundreds lines of similar output, it's a different >> story, a small change in any of the value is much harder to catch, and >> the extra helpful prompt is in fact a burden other than a bless. >> >> (That's why things like spreadsheet is much easier to reader for >> multiple similarly structured data, and in that case it's much better to >> craft a script to convert the lines into a csv) >> >> Unfortunately we don't have the benefit (at least yet) to structure all >> these info into a structured output. >> >> >> But what I'm doing here is to reduce the prompts to minimal (at most 4 >> prompts, "inode", "fileoff", "logical", "physical"), so less duplicated >> strings for multiple lines of similar error messages. >> >> The patchset is in the middle ground between fully detailed output (the >> old one, focusing on single line) and the fully script orient output (no >> prompt at all, just all numbers/strings, focusing on CSV like output). >> >> >> Furthermore, I would also argue that, for entry level end users, they >> can still catch the critical info (like file path and device path) >> without much difficult, and those info is enough for them to take action. >> (E.g. deleting the file, or replace the disk) >> >> Yes, they would get confused on the devid or rootid, but that's fine and >> everyone can learn something new. >> >> For experienced users who understand basics of btrfs, or us developers, >> the split in values are already arranged in a way similar sized values >> are never close together (aka, string/large/small value split). > > Well, I'm a developer and I can tell you if I ever see such log > messages, I'll have to go to the source code to figure out what each > value is supposed to be. > I'll be able to memorize for some hours or even a few days, but after > that I'll forget again and have to look at the source code again. As the core problem is, if you just see one such error, and you need to investigate, yes it needs more digging. But in the real world, the support cases I hit are all have tons of similar lines almost flooding the dmesg (well, in that case they don't include any useful info either). In that case, digging the format once is not a big deal at all, in fact the main heavy lifting part is to determine how severe the corruption is. And I'm not going full csv-like output, we still have several prompts to provide some help. Thanks, Qu > > >> >> Thanks, >> Qu >> >>> >>> Thanks. >>> >
On Thu, Mar 14, 2024 at 8:26 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: > > > > 在 2024/3/15 03:47, Filipe Manana 写道: > > On Thu, Mar 14, 2024 at 9:54 AM Qu Wenruo <wqu@suse.com> wrote: > >> > >> Currently when we increase the device statistics, it would always lead > >> to an error message in the kernel log. > >> > >> I would argue this behavior is not ideal: > >> > >> - It would flood the dmesg and bury real important messages > >> One common scenario is scrub. > >> If scrub hit some errors, it would cause both scrub and > >> btrfs_dev_stat_inc_and_print() to print error messages. > >> > >> And in that case, btrfs_dev_stat_inc_and_print() is completely > >> useless. > >> > >> - The results of btrfs_dev_stat_inc_and_print() is mostly for history > >> monitoring, doesn't has enough details > >> > >> If we trigger the errors during regular read, such messages from > >> btrfs_dev_stat_inc_and_print() won't help us to locate the cause > >> either. > >> > >> The real usage for the btrfs device statistics is for some user space > >> daemon to check if there is any new errors, acting like some checks on > >> SMART, thus we don't really need/want those messages in dmesg. > >> > >> This patch would reduce the log level to debug (disabled by default) for > >> btrfs_dev_stat_inc_and_print(). > >> For users really want to utilize btrfs devices statistics, they should > >> go check "btrfs device stats" periodically, and we should focus the > >> kernel error messages to more important things. > > > > Not sure if this is the right thing to do. > > > > In the scrub context it can be annoying for sure. > > Other cases I'm not so sure about, because having error messages in > > dmesg/syslog may help notice issues more quickly. > > For non-scrub cases, I'd argue we already have enough output: > > No matter if the error is fixed or not, every time a mirror got csum > mismatch or other errors, we already have error message output: > > Data: btrfs_print_data_csum_error() > Meta: btrfs_validate_extent_buffer() > > For repaired ones, we have extra output from bio layer for both metadata > and data: > btrfs_repair_io_failure() > > So I'd say the dev_stat ones are already duplicated. Ok, I suppose it's fine then. Reviewed-by: Filipe Manana <fdmanana@suse.com> Thanks. > > Thanks, > Qu > > > >> > >> CC: stable@vger.kernel.org > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/volumes.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > >> index e49935a54da0..126145950ed3 100644 > >> --- a/fs/btrfs/volumes.c > >> +++ b/fs/btrfs/volumes.c > >> @@ -7828,7 +7828,7 @@ void btrfs_dev_stat_inc_and_print(struct btrfs_device *dev, int index) > >> > >> if (!dev->dev_stats_valid) > >> return; > >> - btrfs_err_rl_in_rcu(dev->fs_info, > >> + btrfs_debug_rl_in_rcu(dev->fs_info, > >> "bdev %s errs: wr %u, rd %u, flush %u, corrupt %u, gen %u", > >> btrfs_dev_name(dev), > >> btrfs_dev_stat_read(dev, BTRFS_DEV_STAT_WRITE_ERRS), > >> -- > >> 2.44.0 > >> > >> > >
在 2024/3/19 02:46, Filipe Manana 写道: > On Thu, Mar 14, 2024 at 8:28 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote: >> >> >> >> 在 2024/3/15 03:56, Filipe Manana 写道: >>> On Thu, Mar 14, 2024 at 9:50 AM Qu Wenruo <wqu@suse.com> wrote: >>>> >>>> The @stripe passed into scrub_stripe_report_errors() either has >>>> stripe->dev and stripe->physical properly populated (regular data >>>> stripes) or stripe->flags would have SCRUB_STRIPE_FLAG_NO_REPORT >>>> (RAID56 P/Q stripes). >>>> >>>> Thus there is no need to go with btrfs_map_block() to get the >>>> dev/physical. >>>> >>>> Just add an extra ASSERT() to make sure we get stripe->dev populated >>>> correctly. >>>> >>>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>>> --- >>>> fs/btrfs/scrub.c | 59 ++++++------------------------------------------ >>>> 1 file changed, 7 insertions(+), 52 deletions(-) >>>> >>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >>>> index 5e371ffdb37b..277583464371 100644 >>>> --- a/fs/btrfs/scrub.c >>>> +++ b/fs/btrfs/scrub.c >>>> @@ -860,10 +860,8 @@ static void scrub_stripe_submit_repair_read(struct scrub_stripe *stripe, >>>> static void scrub_stripe_report_errors(struct scrub_ctx *sctx, >>>> struct scrub_stripe *stripe) >>>> { >>>> - static DEFINE_RATELIMIT_STATE(rs, DEFAULT_RATELIMIT_INTERVAL, >>>> - DEFAULT_RATELIMIT_BURST); >>>> struct btrfs_fs_info *fs_info = sctx->fs_info; >>>> - struct btrfs_device *dev = NULL; >>>> + struct btrfs_device *dev = stripe->dev; >>>> u64 stripe_physical = stripe->physical; >>>> int nr_data_sectors = 0; >>>> int nr_meta_sectors = 0; >>>> @@ -874,35 +872,7 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx, >>>> if (test_bit(SCRUB_STRIPE_FLAG_NO_REPORT, &stripe->state)) >>>> return; >>>> >>>> - /* >>>> - * Init needed infos for error reporting. >>>> - * >>>> - * Although our scrub_stripe infrastructure is mostly based on btrfs_submit_bio() >>>> - * thus no need for dev/physical, error reporting still needs dev and physical. >>>> - */ >>>> - if (!bitmap_empty(&stripe->init_error_bitmap, stripe->nr_sectors)) { >>>> - u64 mapped_len = fs_info->sectorsize; >>>> - struct btrfs_io_context *bioc = NULL; >>>> - int stripe_index = stripe->mirror_num - 1; >>>> - int ret; >>>> - >>>> - /* For scrub, our mirror_num should always start at 1. */ >>>> - ASSERT(stripe->mirror_num >= 1); >>>> - ret = btrfs_map_block(fs_info, BTRFS_MAP_GET_READ_MIRRORS, >>>> - stripe->logical, &mapped_len, &bioc, >>>> - NULL, NULL); >>>> - /* >>>> - * If we failed, dev will be NULL, and later detailed reports >>>> - * will just be skipped. >>>> - */ >>>> - if (ret < 0) >>>> - goto skip; >>>> - stripe_physical = bioc->stripes[stripe_index].physical; >>>> - dev = bioc->stripes[stripe_index].dev; >>>> - btrfs_put_bioc(bioc); >>>> - } >>>> - >>>> -skip: >>>> + ASSERT(dev); >>>> for_each_set_bit(sector_nr, &stripe->extent_sector_bitmap, stripe->nr_sectors) { >>>> u64 logical = stripe->logical + >>>> (sector_nr << fs_info->sectorsize_bits); >>>> @@ -933,42 +903,27 @@ static void scrub_stripe_report_errors(struct scrub_ctx *sctx, >>>> * output the message of repaired message. >>>> */ >>>> if (repaired) { >>>> - if (dev) { >>>> - btrfs_err_rl_in_rcu(fs_info, >>>> + btrfs_err_rl_in_rcu(fs_info, >>>> "fixed up error at logical %llu on dev %s physical %llu", >>>> logical, btrfs_dev_name(dev), >>>> physical); >>>> - } else { >>>> - btrfs_err_rl_in_rcu(fs_info, >>>> - "fixed up error at logical %llu on mirror %u", >>>> - logical, stripe->mirror_num); >>>> - } >>>> continue; >>>> } >>>> >>>> /* The remaining are all for unrepaired. */ >>>> - if (dev) { >>>> - btrfs_err_rl_in_rcu(fs_info, >>>> + btrfs_err_rl_in_rcu(fs_info, >>>> "unable to fixup (regular) error at logical %llu on dev %s physical %llu", >>>> logical, btrfs_dev_name(dev), >>>> physical); >>>> - } else { >>>> - btrfs_err_rl_in_rcu(fs_info, >>>> - "unable to fixup (regular) error at logical %llu on mirror %u", >>>> - logical, stripe->mirror_num); >>>> - } >>>> >>>> if (test_bit(sector_nr, &stripe->io_error_bitmap)) >>>> - if (__ratelimit(&rs) && dev) >>>> - scrub_print_common_warning("i/o error", dev, >>>> + scrub_print_common_warning("i/o error", dev, >>>> logical, physical); >>>> if (test_bit(sector_nr, &stripe->csum_error_bitmap)) >>>> - if (__ratelimit(&rs) && dev) >>>> - scrub_print_common_warning("checksum error", dev, >>>> + scrub_print_common_warning("checksum error", dev, >>>> logical, physical); >>>> if (test_bit(sector_nr, &stripe->meta_error_bitmap)) >>>> - if (__ratelimit(&rs) && dev) >>>> - scrub_print_common_warning("header error", dev, >>>> + scrub_print_common_warning("header error", dev, >>> >>> Why are we removing the rate limiting? >>> This seems like an unrelated change. >> >> Because the ratelimit is not consistent. >> >> E.g. the "fixed up error" line is not rated limited by @rs, but by >> btrfs_err_rl_in_rcu(). > > That I don't understand. > > What I see is that scrub_print_common_warning() calls > btrfs_warn_in_rcu() or btrfs_warn() or scrub_print_warning_inode() > (which calls btrfs_warn_in_rcu()). > I don't see where btrfs_err_rl_in_rcu() is called. So to me it seems > @rs is doing the rate limiting. > >> >> And we would have scrub_print_common_warning() to go btrfs_*_rl() helper >> in the coming patches. > > Ok, but if @rs is indeed doing the rate limiting here, then from an > organization point of view, it should be removed in the later patch > that makes scrub_print_common_warning() use the btrfs_*_rl() helpers. OK, I'll rearrange the patches so that @rs would only be removed after we have migrate all the messages are migrated to rate limited versions. Thanks, Qu > > Thanks. > >> >> Thanks, >> Qu >>> >>> Everything else looks ok. >>> >>> Thanks. >>> >>>> logical, physical); >>>> } >>>> >>>> -- >>>> 2.44.0 >>>> >>>> >>>
On Mon, Mar 18, 2024 at 12:14:54PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> Trivial stuff, details in the change logs.
>
> Filipe Manana (2):
> btrfs: remove pointless readahead callback wrapper
> btrfs: remove pointless writepages callback wrapper
Reviewed-by: David Sterba <dsterba@suse.com>
On Mon, Mar 18, 2024 at 10:13:13AM +0530, Anand Jain wrote:
> There are reports that since version 6.7 update-grub fails to find the
> device of the root on systems without initrd and on a single device.
>
> This looks like the device name changed in the output of
> /proc/self/mountinfo:
>
> 6.5-rc5 working
>
> 18 1 0:16 / / rw,noatime - btrfs /dev/sda8 ...
>
> 6.7 not working:
>
> 17 1 0:15 / / rw,noatime - btrfs /dev/root ...
>
> and "update-grub" shows this error:
>
> /usr/sbin/grub-probe: error: cannot find a device for / (is /dev mounted?)
>
> This looks like it's related to the device name, but grub-probe
> recognizes the "/dev/root" path and tries to find the underlying device.
> However there's a special case for some filesystems, for btrfs in
> particular.
>
> The generic root device detection heuristic is not done and it all
> relies on reading the device infos by a btrfs specific ioctl. This ioctl
> returns the device name as it was saved at the time of device scan (in
> this case it's /dev/root).
>
> The change in 6.7 for temp_fsid to allow several single device
> filesystem to exist with the same fsid (and transparently generate a new
> UUID at mount time) was to skip caching/registering such devices.
>
> This also skipped mounted device. One step of scanning is to check if
> the device name hasn't changed, and if yes then update the cached value.
>
> This broke the grub-probe as it always read the device /dev/root and
> couldn't find it in the system. A temporary workaround is to create a
> symlink but this does not survive reboot.
>
> The right fix is to allow updating the device path of a mounted
> filesystem even if this is a single device one.
>
> In the fix, check if the device's major:minor number matches with the
> cached device. If they do, then we can allow the scan to happen so that
> device_list_add() can take care of updating the device path. The file
> descriptor remains unchanged.
>
> This does not affect the temp_fsid feature, the UUID of the mounted
> filesystem remains the same and the matching is based on device major:minor
> which is unique per mounted filesystem.
>
> This covers the path when the device (that exists for all mounted
> devices) name changes, updating /dev/root to /dev/sdx. Any other single
> device with filesystem and is not mounted is still skipped.
>
> Note that if a system is booted and initial mount is done on the
> /dev/root device, this will be the cached name of the device. Only after
> the command "btrfs device scan" it will change as it triggers the
> rename.
>
> The fix was verified by users whose systems were affected.
>
> CC: stable@vger.kernel.org # 6.7+
> Fixes: bc27d6f0aa0e ("btrfs: scan but don't register device on single device filesystem")
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=218353
> Link: https://lore.kernel.org/lkml/CAKLYgeJ1tUuqLcsquwuFqjDXPSJpEiokrWK2gisPKDZLs8Y2TQ@mail.gmail.com/
> Tested-by: Alex Romosan <aromosan@gmail.com>
> Tested-by: CHECK_1234543212345@protonmail.com
> Reviewed-by: David Sterba <dsterba@suse.com>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
> v5:
> Fix the linux-next build failure reported here:
> https://lore.kernel.org/all/20240318091755.1d0f696f@canb.auug.org.au/
> As the Linux-next branch no longer has the this commit,
> I've sent out the entire patch again.
Thanks but this won't work. The code in v5 is against current master
with bdev_handle -> file_bdev change but the patch gets merged from
branch that does not have that.
For backport to stable we'll need the v4 version while for merge to
master (in the next pull request) it's going to be v5.
I'll do a separate pull request with this change on the correct base and
we'll have to let stable team know which patch to pick.
On Thu, Mar 14, 2024 at 8:56 PM Qu Wenruo <wqu@suse.com> wrote: > > > > 在 2024/3/15 04:10, Filipe Manana 写道: > > On Thu, Mar 14, 2024 at 9:51 AM Qu Wenruo <wqu@suse.com> wrote: > >> > >> Currently the scrub error report is pretty long: > >> > >> BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872 on dev /dev/mapper/test-scratch1 physical 13647872 > >> BTRFS warning (device dm-2): checksum error at logical 13647872 on dev /dev/mapper/test-scratch1, physical 13647872, root 5, inode 257, offset 16384, length 4096, links 1 (path: file1) > >> > >> Since we have so many things to output, it's not a surprise we got long > >> error lines. > >> > >> To make the lines a little shorter, and make important info more > >> obvious, change the unify output to something like this: > >> > >> BTRFS error (device dm-2): unable to fixup (regular) error at logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872 > >> BTRFS warning (device dm-2): checksum error at inode 5/257(file1) fileoff 16384, logical 13647872(1) physical 1(/dev/mapper/test-scratch1)13647872 > > > > I find that hard to read because: > > > > 1) Please leave spaces before opening parenthesis. > > That makes things less cluttered and easier to the eyes, more > > consistent with what we generally do and it's the formal way to do > > things in English (see > > https://www.scribens.com/grammar-rules/typography/spacing.html); > > Yep, I can do that, but I also want to keep the involved members > together thus closer. > > Not sure if adding spaces would still keep the close relationships > between the values. > > E.g: inode 5/256 (file1) fileoff 16384, logical 123456 (1) physical 1 > (scratch1) 123456 > > It makes it a little harder to indicate that "(1)" is related to logical > address (thus mirror number). > > > > > 2) Instead of "inode 5/257(file1)", something a lot easier to > > understand like "inode 5 root 257 path \"file1\"", which leaves no > > margin for doubt about what each number is; > > > > 3) Same with "logical 13647872(1)" - what is the 1? That will be the > > question anyone reading such a log message will likely have. > > Something like "logical 13647872 mirror 1" makes it clear; > > > > 4) Same with "physical 1(/dev/mapper/test-scratch1)13647872". > > Something like "physical 13647872 device ID 1 device path > > \"/dev/mapper/test-scratch1\"", makes it clear what each number is and > > easier to read. > > I totally understand the original output format is much easier to read > on its own. > > However if we have hundreds lines of similar output, it's a different > story, a small change in any of the value is much harder to catch, and > the extra helpful prompt is in fact a burden other than a bless. > > (That's why things like spreadsheet is much easier to reader for > multiple similarly structured data, and in that case it's much better to > craft a script to convert the lines into a csv) > > Unfortunately we don't have the benefit (at least yet) to structure all > these info into a structured output. > > > But what I'm doing here is to reduce the prompts to minimal (at most 4 > prompts, "inode", "fileoff", "logical", "physical"), so less duplicated > strings for multiple lines of similar error messages. > > The patchset is in the middle ground between fully detailed output (the > old one, focusing on single line) and the fully script orient output (no > prompt at all, just all numbers/strings, focusing on CSV like output). > > > Furthermore, I would also argue that, for entry level end users, they > can still catch the critical info (like file path and device path) > without much difficult, and those info is enough for them to take action. > (E.g. deleting the file, or replace the disk) > > Yes, they would get confused on the devid or rootid, but that's fine and > everyone can learn something new. > > For experienced users who understand basics of btrfs, or us developers, > the split in values are already arranged in a way similar sized values > are never close together (aka, string/large/small value split). Well, I'm a developer and I can tell you if I ever see such log messages, I'll have to go to the source code to figure out what each value is supposed to be. I'll be able to memorize for some hours or even a few days, but after that I'll forget again and have to look at the source code again. > > Thanks, > Qu > > > > > Thanks. > >