| Message ID | 1462786798-15247-1-git-send-email-dsterba@suse.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On 05/09/2016 05:39 PM, David Sterba wrote: > Currently we lack the identification of the filesystem in most if not > all mount messages, done via printk/pr_* functions. We can use the > btrfs_* helpers in open_ctree, as the fs_info <-> sb link is established > at the beginning of the function. While here I also recommend to pass fs_devices instead of fs_info to btrfs_printk(). That's mainly because before the fs is mounted we don't have fs_info, however fs_devices exists in both the mounted and unmount context. If you agree, I could send a patch on top of your patch. Otherwise the rest below looks fine. Thanks, Anand. > The messages have been updated at the same time to be more consistent: > > * dropped sb->s_id, as it's not available via btrfs_* > * added %d for return code where appropriate > * wording changed > * %Lx replaced by %llx > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/disk-io.c | 102 ++++++++++++++++++++++++++--------------------------- > 1 file changed, 50 insertions(+), 52 deletions(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index 4e47849d7427..cc8aee26d17b 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2713,7 +2713,7 @@ int open_ctree(struct super_block *sb, > * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k). > */ > if (btrfs_check_super_csum(bh->b_data)) { > - printk(KERN_ERR "BTRFS: superblock checksum mismatch\n"); > + btrfs_err(fs_info, "superblock checksum mismatch"); > err = -EINVAL; > brelse(bh); > goto fail_alloc; > @@ -2733,7 +2733,7 @@ int open_ctree(struct super_block *sb, > > ret = btrfs_check_super_valid(fs_info, sb->s_flags & MS_RDONLY); > if (ret) { > - printk(KERN_ERR "BTRFS: superblock contains fatal errors\n"); > + btrfs_err(fs_info, "superblock contains fatal errors"); > err = -EINVAL; > goto fail_alloc; > } > @@ -2768,9 +2768,9 @@ int open_ctree(struct super_block *sb, > features = btrfs_super_incompat_flags(disk_super) & > ~BTRFS_FEATURE_INCOMPAT_SUPP; > if (features) { > - printk(KERN_ERR "BTRFS: couldn't mount because of " > - "unsupported optional features (%Lx).\n", > - features); > + btrfs_err(fs_info, > + "cannot mount because of unsupported optional features (%llx)", > + features); > err = -EINVAL; > goto fail_alloc; > } > @@ -2781,7 +2781,7 @@ int open_ctree(struct super_block *sb, > features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO; > > if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) > - printk(KERN_INFO "BTRFS: has skinny extents\n"); > + btrfs_info(fs_info, "has skinny extents"); > > /* > * flag our filesystem as having big metadata blocks if > @@ -2789,7 +2789,8 @@ int open_ctree(struct super_block *sb, > */ > if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { > if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) > - printk(KERN_INFO "BTRFS: flagging fs with big metadata feature\n"); > + btrfs_info(fs_info, > + "flagging fs with big metadata feature"); > features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; > } > > @@ -2805,9 +2806,9 @@ int open_ctree(struct super_block *sb, > */ > if ((features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS) && > (sectorsize != nodesize)) { > - printk(KERN_ERR "BTRFS: unequal leaf/node/sector sizes " > - "are not allowed for mixed block groups on %s\n", > - sb->s_id); > + btrfs_err(fs_info, > +"unequal nodesize/sectorsize (%u != %u) are not allowed for mixed block groups", > + nodesize, sectorsize); > goto fail_alloc; > } > > @@ -2820,8 +2821,8 @@ int open_ctree(struct super_block *sb, > features = btrfs_super_compat_ro_flags(disk_super) & > ~BTRFS_FEATURE_COMPAT_RO_SUPP; > if (!(sb->s_flags & MS_RDONLY) && features) { > - printk(KERN_ERR "BTRFS: couldn't mount RDWR because of " > - "unsupported option features (%Lx).\n", > + btrfs_err(fs_info, > + "cannot mount read-write because of unsupported optional features (%llx)", > features); > err = -EINVAL; > goto fail_alloc; > @@ -2850,8 +2851,7 @@ int open_ctree(struct super_block *sb, > ret = btrfs_read_sys_array(tree_root); > mutex_unlock(&fs_info->chunk_mutex); > if (ret) { > - printk(KERN_ERR "BTRFS: failed to read the system " > - "array on %s\n", sb->s_id); > + btrfs_err(fs_info, "failed to read the system array: %d", ret); > goto fail_sb_buffer; > } > > @@ -2865,8 +2865,7 @@ int open_ctree(struct super_block *sb, > generation); > if (IS_ERR(chunk_root->node) || > !extent_buffer_uptodate(chunk_root->node)) { > - printk(KERN_ERR "BTRFS: failed to read chunk root on %s\n", > - sb->s_id); > + btrfs_err(fs_info, "failed to read chunk root"); > if (!IS_ERR(chunk_root->node)) > free_extent_buffer(chunk_root->node); > chunk_root->node = NULL; > @@ -2880,8 +2879,7 @@ int open_ctree(struct super_block *sb, > > ret = btrfs_read_chunk_tree(chunk_root); > if (ret) { > - printk(KERN_ERR "BTRFS: failed to read chunk tree on %s\n", > - sb->s_id); > + btrfs_err(fs_info, "failed to read chunk tree: %d", ret); > goto fail_tree_roots; > } > > @@ -2892,8 +2890,7 @@ int open_ctree(struct super_block *sb, > btrfs_close_extra_devices(fs_devices, 0); > > if (!fs_devices->latest_bdev) { > - printk(KERN_ERR "BTRFS: failed to read devices on %s\n", > - sb->s_id); > + btrfs_err(fs_info, "failed to read devices"); > goto fail_tree_roots; > } > > @@ -2905,8 +2902,7 @@ int open_ctree(struct super_block *sb, > generation); > if (IS_ERR(tree_root->node) || > !extent_buffer_uptodate(tree_root->node)) { > - printk(KERN_WARNING "BTRFS: failed to read tree root on %s\n", > - sb->s_id); > + btrfs_warn(fs_info, "failed to read tree root"); > if (!IS_ERR(tree_root->node)) > free_extent_buffer(tree_root->node); > tree_root->node = NULL; > @@ -2938,20 +2934,19 @@ int open_ctree(struct super_block *sb, > > ret = btrfs_recover_balance(fs_info); > if (ret) { > - printk(KERN_ERR "BTRFS: failed to recover balance\n"); > + btrfs_err(fs_info, "failed to recover balance: %d", ret); > goto fail_block_groups; > } > > ret = btrfs_init_dev_stats(fs_info); > if (ret) { > - printk(KERN_ERR "BTRFS: failed to init dev_stats: %d\n", > - ret); > + btrfs_err(fs_info, "failed to init dev_stats: %d", ret); > goto fail_block_groups; > } > > ret = btrfs_init_dev_replace(fs_info); > if (ret) { > - pr_err("BTRFS: failed to init dev_replace: %d\n", ret); > + btrfs_err(fs_info, "failed to init dev_replace: %d", ret); > goto fail_block_groups; > } > > @@ -2959,31 +2954,33 @@ int open_ctree(struct super_block *sb, > > ret = btrfs_sysfs_add_fsid(fs_devices, NULL); > if (ret) { > - pr_err("BTRFS: failed to init sysfs fsid interface: %d\n", ret); > + btrfs_err(fs_info, "failed to init sysfs fsid interface: %d", > + ret); > goto fail_block_groups; > } > > ret = btrfs_sysfs_add_device(fs_devices); > if (ret) { > - pr_err("BTRFS: failed to init sysfs device interface: %d\n", ret); > + btrfs_err(fs_info, "failed to init sysfs device interface: %d", > + ret); > goto fail_fsdev_sysfs; > } > > ret = btrfs_sysfs_add_mounted(fs_info); > if (ret) { > - pr_err("BTRFS: failed to init sysfs interface: %d\n", ret); > + btrfs_err(fs_info, "failed to init sysfs interface: %d", ret); > goto fail_fsdev_sysfs; > } > > ret = btrfs_init_space_info(fs_info); > if (ret) { > - printk(KERN_ERR "BTRFS: Failed to initial space info: %d\n", ret); > + btrfs_err(fs_info, "failed to initialize space info: %d", ret); > goto fail_sysfs; > } > > ret = btrfs_read_block_groups(fs_info->extent_root); > if (ret) { > - printk(KERN_ERR "BTRFS: Failed to read block groups: %d\n", ret); > + btrfs_err(fs_info, "failed to read block groups: %d", ret); > goto fail_sysfs; > } > fs_info->num_tolerated_disk_barrier_failures = > @@ -2991,7 +2988,8 @@ int open_ctree(struct super_block *sb, > if (fs_info->fs_devices->missing_devices > > fs_info->num_tolerated_disk_barrier_failures && > !(sb->s_flags & MS_RDONLY)) { > - pr_warn("BTRFS: missing devices(%llu) exceeds the limit(%d), writeable mount is not allowed\n", > + btrfs_warn(fs_info, > +"missing devices (%llu) exceeds the limit (%d), writeable mount is not allowed", > fs_info->fs_devices->missing_devices, > fs_info->num_tolerated_disk_barrier_failures); > goto fail_sysfs; > @@ -3011,8 +3009,7 @@ int open_ctree(struct super_block *sb, > if (!btrfs_test_opt(tree_root, SSD) && > !btrfs_test_opt(tree_root, NOSSD) && > !fs_info->fs_devices->rotating) { > - printk(KERN_INFO "BTRFS: detected SSD devices, enabling SSD " > - "mode\n"); > + btrfs_info(fs_info, "detected SSD devices, enabling SSD mode"); > btrfs_set_opt(fs_info->mount_opt, SSD); > } > > @@ -3030,8 +3027,9 @@ int open_ctree(struct super_block *sb, > 1 : 0, > fs_info->check_integrity_print_mask); > if (ret) > - printk(KERN_WARNING "BTRFS: failed to initialize" > - " integrity check module %s\n", sb->s_id); > + btrfs_warn(fs_info, > + "failed to initialize integrity check module: %d", > + ret); > } > #endif > ret = btrfs_read_qgroup_config(fs_info); > @@ -3061,8 +3059,8 @@ int open_ctree(struct super_block *sb, > ret = btrfs_recover_relocation(tree_root); > mutex_unlock(&fs_info->cleaner_mutex); > if (ret < 0) { > - printk(KERN_WARNING > - "BTRFS: failed to recover relocation\n"); > + btrfs_warn(fs_info, "failed to recover relocation: %d", > + ret); > err = -EINVAL; > goto fail_qgroup; > } > @@ -3083,11 +3081,11 @@ int open_ctree(struct super_block *sb, > > if (btrfs_test_opt(tree_root, FREE_SPACE_TREE) && > !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) { > - pr_info("BTRFS: creating free space tree\n"); > + btrfs_info(fs_info, "creating free space tree"); > ret = btrfs_create_free_space_tree(fs_info); > if (ret) { > - pr_warn("BTRFS: failed to create free space tree %d\n", > - ret); > + btrfs_warn(fs_info, > + "failed to create free space tree: %d", ret); > close_ctree(tree_root); > return ret; > } > @@ -3104,14 +3102,14 @@ int open_ctree(struct super_block *sb, > > ret = btrfs_resume_balance_async(fs_info); > if (ret) { > - printk(KERN_WARNING "BTRFS: failed to resume balance\n"); > + btrfs_warn(fs_info, "failed to resume balance: %d", ret); > close_ctree(tree_root); > return ret; > } > > ret = btrfs_resume_dev_replace_async(fs_info); > if (ret) { > - pr_warn("BTRFS: failed to resume dev_replace\n"); > + btrfs_warn(fs_info, "failed to resume device replace: %d", ret); > close_ctree(tree_root); > return ret; > } > @@ -3120,33 +3118,33 @@ int open_ctree(struct super_block *sb, > > if (btrfs_test_opt(tree_root, CLEAR_CACHE) && > btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) { > - pr_info("BTRFS: clearing free space tree\n"); > + btrfs_info(fs_info, "clearing free space tree"); > ret = btrfs_clear_free_space_tree(fs_info); > if (ret) { > - pr_warn("BTRFS: failed to clear free space tree %d\n", > - ret); > + btrfs_warn(fs_info, > + "failed to clear free space tree: %d", ret); > close_ctree(tree_root); > return ret; > } > } > > if (!fs_info->uuid_root) { > - pr_info("BTRFS: creating UUID tree\n"); > + btrfs_info(fs_info, "creating UUID tree"); > ret = btrfs_create_uuid_tree(fs_info); > if (ret) { > - pr_warn("BTRFS: failed to create the UUID tree %d\n", > - ret); > + btrfs_warn(fs_info, > + "failed to create the UUID tree: %d", ret); > close_ctree(tree_root); > return ret; > } > } else if (btrfs_test_opt(tree_root, RESCAN_UUID_TREE) || > fs_info->generation != > btrfs_super_uuid_tree_generation(disk_super)) { > - pr_info("BTRFS: checking UUID tree\n"); > + btrfs_info(fs_info, "checking UUID tree"); > ret = btrfs_check_uuid_tree(fs_info); > if (ret) { > - pr_warn("BTRFS: failed to check the UUID tree %d\n", > - ret); > + btrfs_warn(fs_info, > + "failed to check the UUID tree: %d", ret); > close_ctree(tree_root); > return ret; > } > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2016 at 10:34:19AM +0800, Anand Jain wrote: > On 05/09/2016 05:39 PM, David Sterba wrote: > > Currently we lack the identification of the filesystem in most if not > > all mount messages, done via printk/pr_* functions. We can use the > > btrfs_* helpers in open_ctree, as the fs_info <-> sb link is established > > at the beginning of the function. > > While here I also recommend to pass fs_devices instead of fs_info > to btrfs_printk(). That's mainly because before the fs is mounted > we don't have fs_info, however fs_devices exists in both the mounted > and unmount context. If you agree, I could send a patch on top of > your patch. Yeah, before we mount we don't have fs_info. My idea was to provide another set of functions that would take something else than fs_info to print the filesystem identifier. Switching btrfs_err & others to fs_devices everywhere would be too invasive. In more detail: * introduce _btrfs_printk that takes a string pointer as 1st argument (this could be used for messages before fs_info exists) * _btrfs_printk(NULL, ...) will be valid * then btrfs_printk(fs_info, ...) will become a wrapper _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) * _btrfs_err & others do not need to be introduced, we can use the standard KERN_ERR > Otherwise the rest below looks fine. Thnaks. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2016 03:42 PM, David Sterba wrote: > On Tue, May 10, 2016 at 10:34:19AM +0800, Anand Jain wrote: >> On 05/09/2016 05:39 PM, David Sterba wrote: >>> Currently we lack the identification of the filesystem in most if not >>> all mount messages, done via printk/pr_* functions. We can use the >>> btrfs_* helpers in open_ctree, as the fs_info <-> sb link is established >>> at the beginning of the function. >> >> While here I also recommend to pass fs_devices instead of fs_info >> to btrfs_printk(). That's mainly because before the fs is mounted >> we don't have fs_info, however fs_devices exists in both the mounted >> and unmount context. If you agree, I could send a patch on top of >> your patch. > > Yeah, before we mount we don't have fs_info. My idea was to provide > another set of functions that would take something else than fs_info to > print the filesystem identifier. > > Switching btrfs_err & others to fs_devices everywhere would be too > invasive. right. > In more detail: > > * introduce _btrfs_printk that takes a string pointer as 1st argument > (this could be used for messages before fs_info exists) > * _btrfs_printk(NULL, ...) will be valid > * then btrfs_printk(fs_info, ...) will become a wrapper > _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) > * _btrfs_err & others do not need to be introduced, we can use the > standard KERN_ERR Does it mean logs when fs_info is not available won't have fsid ? Except for the logs in the context such as modload.. which does not involve a disk. Consistency in logging would help. Like fishing-out all logs related to a FSID. Thanks, Anand >> Otherwise the rest below looks fine. > > Thnaks. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2016 at 04:01:05PM +0800, Anand Jain wrote: > > In more detail: > > > > * introduce _btrfs_printk that takes a string pointer as 1st argument > > (this could be used for messages before fs_info exists) > > * _btrfs_printk(NULL, ...) will be valid > > * then btrfs_printk(fs_info, ...) will become a wrapper > > _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) > > * _btrfs_err & others do not need to be introduced, we can use the > > standard KERN_ERR > > Does it mean logs when fs_info is not available won't have fsid ? Right, we don't have fsid until we read the superblock from disk (somewhere in open_ctree). > Except for the logs in the context such as modload.. which does not > involve a disk. Consistency in logging would help. Like fishing-out > all logs related to a FSID. Yeah, this is probably related to your patches switching the messages to print fsid instead of device. Consistency is desirable of course, though we might need to make the output style configurable. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2016 04:21 PM, David Sterba wrote: > On Tue, May 10, 2016 at 04:01:05PM +0800, Anand Jain wrote: >>> In more detail: >>> >>> * introduce _btrfs_printk that takes a string pointer as 1st argument >>> (this could be used for messages before fs_info exists) >>> * _btrfs_printk(NULL, ...) will be valid >>> * then btrfs_printk(fs_info, ...) will become a wrapper >>> _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) >>> * _btrfs_err & others do not need to be introduced, we can use the >>> standard KERN_ERR >> >> Does it mean logs when fs_info is not available won't have fsid ? > > Right, we don't have fsid until we read the superblock from disk > (somewhere in open_ctree). Not only at open_ctree. We create fs_devices with fsid when 'btrfs dev scan' and we do populate btrfs_fs_devices->fsid this is well before open_ctree is called. OR open_ctree may not be called at all in some cases. >> Except for the logs in the context such as modload.. which does not >> involve a disk. Consistency in logging would help. Like fishing-out >> all logs related to a FSID. > > Yeah, this is probably related to your patches switching the messages to > print fsid instead of device. Consistency is desirable of course, though > we might need to make the output style configurable. If there is something that works simple, better. I am fine. Generally servers may have more than one fs mounted. So filter by fsid comes handy. Without worrying about when it was labeled, and troubleshooting scripts to fail. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2016 at 04:40:58PM +0800, Anand Jain wrote: > > > On 05/10/2016 04:21 PM, David Sterba wrote: > > On Tue, May 10, 2016 at 04:01:05PM +0800, Anand Jain wrote: > >>> In more detail: > >>> > >>> * introduce _btrfs_printk that takes a string pointer as 1st argument > >>> (this could be used for messages before fs_info exists) > >>> * _btrfs_printk(NULL, ...) will be valid > >>> * then btrfs_printk(fs_info, ...) will become a wrapper > >>> _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) > >>> * _btrfs_err & others do not need to be introduced, we can use the > >>> standard KERN_ERR > >> > >> Does it mean logs when fs_info is not available won't have fsid ? > > > > Right, we don't have fsid until we read the superblock from disk > > (somewhere in open_ctree). > > Not only at open_ctree. We create fs_devices with fsid when 'btrfs dev > scan' and we do populate btrfs_fs_devices->fsid this is well before > open_ctree is called. OR open_ctree may not be called at all in some > cases. So in this case we'd need to transform the fsid (from any source) to the string before passing to the proposed _btrfs_printk. > >> Except for the logs in the context such as modload.. which does not > >> involve a disk. Consistency in logging would help. Like fishing-out > >> all logs related to a FSID. > > > > Yeah, this is probably related to your patches switching the messages to > > print fsid instead of device. Consistency is desirable of course, though > > we might need to make the output style configurable. > > If there is something that works simple, better. I am fine. > > Generally servers may have more than one fs mounted. So filter > by fsid comes handy. Without worrying about when it was labeled, > and troubleshooting scripts to fail. The idea is: mount -o log=fsid /dev/sda /mnt/path where the message will print the fsid in place of device: BTRFS info (uuid bcf91f8e-03bb-4e90-9546-24f0564c92a1): disk space caching is enabled also could be tunable after mount via a sysfs file. Possibly we can also allow to print the label. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2016 10:28 PM, David Sterba wrote: > On Tue, May 10, 2016 at 04:40:58PM +0800, Anand Jain wrote: >> >> >> On 05/10/2016 04:21 PM, David Sterba wrote: >>> On Tue, May 10, 2016 at 04:01:05PM +0800, Anand Jain wrote: >>>>> In more detail: >>>>> >>>>> * introduce _btrfs_printk that takes a string pointer as 1st argument >>>>> (this could be used for messages before fs_info exists) >>>>> * _btrfs_printk(NULL, ...) will be valid >>>>> * then btrfs_printk(fs_info, ...) will become a wrapper >>>>> _btrfs_printk(btrfs_sb(fs_info)->s_id, ...) >>>>> * _btrfs_err & others do not need to be introduced, we can use the >>>>> standard KERN_ERR >>>> >>>> Does it mean logs when fs_info is not available won't have fsid ? >>> >>> Right, we don't have fsid until we read the superblock from disk >>> (somewhere in open_ctree). >> >> Not only at open_ctree. We create fs_devices with fsid when 'btrfs dev >> scan' and we do populate btrfs_fs_devices->fsid this is well before >> open_ctree is called. OR open_ctree may not be called at all in some >> cases. > > So in this case we'd need to transform the fsid (from any source) to the > string before passing to the proposed _btrfs_printk. > >>>> Except for the logs in the context such as modload.. which does not >>>> involve a disk. Consistency in logging would help. Like fishing-out >>>> all logs related to a FSID. >>> >>> Yeah, this is probably related to your patches switching the messages to >>> print fsid instead of device. Consistency is desirable of course, though >>> we might need to make the output style configurable. >> >> If there is something that works simple, better. I am fine. >> >> Generally servers may have more than one fs mounted. So filter >> by fsid comes handy. Without worrying about when it was labeled, >> and troubleshooting scripts to fail. > > The idea is: > > mount -o log=fsid /dev/sda /mnt/path This makes life of a 3rd party troubleshooter more difficult. As it needs to be understood what mount option was used, and if its not changed in between by the customer, and if the logs by filter won't show some of the critical logs, would lead to a wrong analysis of the issue. But whats the concern for print FSID by default (without having to come through the -o log=fsid) ? Just my understanding: For real end users we need to provide everything at the cli output. That is without asking them to refer to dmesg in the cli out put. IMO. (I could be wrong). Troubleshooters are the people looking at dmesg. So finding the FSID can be expected ? Further there is nothing avoids user not to label two FS with the same label. Thanks, -Anand > where the message will print the fsid in place of device: > > BTRFS info (uuid bcf91f8e-03bb-4e90-9546-24f0564c92a1): disk space caching is enabled > > also could be tunable after mount via a sysfs file. Possibly we can also > allow to print the label. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 10, 2016 at 11:00:01PM +0800, Anand Jain wrote: > >> If there is something that works simple, better. I am fine. > >> > >> Generally servers may have more than one fs mounted. So filter > >> by fsid comes handy. Without worrying about when it was labeled, > >> and troubleshooting scripts to fail. > > > > The idea is: > > > > mount -o log=fsid /dev/sda /mnt/path > > > This makes life of a 3rd party troubleshooter more difficult. As it > needs to be understood what mount option was used, and if its not > changed in between by the customer, and if the logs by filter won't > show some of the critical logs, would lead to a wrong analysis of > the issue. > > But whats the concern for print FSID by default (without having to > come through the -o log=fsid) ? Concerns as I see them: * current default is to print the device name -- so this would mean a forced change in defaults * the FSID is not very human friendly. I can remember several device names and I'll probably know which filesystem is on which * the FSID is very long, consumes half of the line before the actual message starts I understand the benefits of automated filtering by FSID, but we have conflicting interests here. If you're going to deploy automated log scanning, you probably have automated the filesystem mkfs & mount, so it's a matter of configuration to get it done and in one place. > Just my understanding: > For real end users we need to provide everything at the cli output. > That is without asking them to refer to dmesg in the cli out put. IMO. > (I could be wrong). Troubleshooters are the people looking at dmesg. > So finding the FSID can be expected ? I don't think that looking up device names is making troubleshooting significantly harder and never found it to be a problem in my past experienes. Besides, errors from lower layers report the device names, so bad sectors or failed writes pop up very quickly in the searches. Either way, I'm willing to make it configurable so it addresses all usecases. Another way came to my mind: make it a module parameter, so even the mount option or sysfs settings is not needed and the defaults are system-wide. > Further there is nothing avoids user not to label two FS with the > same label. That's true and users' responsibility not to shoot themselves. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/2016 06:56 PM, David Sterba wrote: > On Tue, May 10, 2016 at 11:00:01PM +0800, Anand Jain wrote: >>>> If there is something that works simple, better. I am fine. >>>> >>>> Generally servers may have more than one fs mounted. So filter >>>> by fsid comes handy. Without worrying about when it was labeled, >>>> and troubleshooting scripts to fail. >>> >>> The idea is: >>> >>> mount -o log=fsid /dev/sda /mnt/path >> >> >> This makes life of a 3rd party troubleshooter more difficult. As it >> needs to be understood what mount option was used, and if its not >> changed in between by the customer, and if the logs by filter won't >> show some of the critical logs, would lead to a wrong analysis of >> the issue. >> >> But whats the concern for print FSID by default (without having to >> come through the -o log=fsid) ? > > Concerns as I see them: > * current default is to print the device name -- so this would mean a > forced change in defaults > * the FSID is not very human friendly. I can remember several device > names and I'll probably know which filesystem is on which > * the FSID is very long, consumes half of the line before the actual > message starts Yes, indeed. I think we discussed that there isn't any other alternative as well. Also I was thinking something like in the 'git log --online' output but that might conflict in some cases ? (recently there was a progs patch which fixed the short fsid hash conflict). > I understand the benefits of automated filtering by FSID, but we have > conflicting interests here. If you're going to deploy automated log > scanning, you probably have automated the filesystem mkfs & mount, so > it's a matter of configuration to get it done and in one place. Makes sense. Actually automated scripts wasn't in my discussion, I was thinking of support engineers debugging customer issues using hand written scripts. Probably we could taken an experimental project, to have these logs in cvs format, and use external scripts to monitor and debug. Make something like splunk.com an easy job. >> Just my understanding: >> For real end users we need to provide everything at the cli output. >> That is without asking them to refer to dmesg in the cli out put. IMO. >> (I could be wrong). Troubleshooters are the people looking at dmesg. >> So finding the FSID can be expected ? > > I don't think that looking up device names is making troubleshooting > significantly harder and never found it to be a problem in my past > experienes. > > Besides, errors from lower layers report the device names, so bad > sectors or failed writes pop up very quickly in the searches. Right. FSID won't provide that advantage. > Either way, I'm willing to make it configurable so it addresses all > usecases. > Another way came to my mind: make it a module parameter, so > even the mount option or sysfs settings is not needed and the defaults > are system-wide. Yep. This helps. I was thinking from the angle of support engineers, who would solve customers issues (it happens at most of the Linux vendors), They would write/run scripts on their own to understand the problem from the kernel logs. So now it can be assured that logs format would remain same per distribution/vendor. Thanks for taking time to explain. - Anand >> Further there is nothing avoids user not to label two FS with the >> same label. > > That's true and users' responsibility not to shoot themselves. > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 11, 2016 at 07:55:57PM +0800, Anand Jain wrote: > > * current default is to print the device name -- so this would mean a > > forced change in defaults > > * the FSID is not very human friendly. I can remember several device > > names and I'll probably know which filesystem is on which > > * the FSID is very long, consumes half of the line before the actual > > message starts > > Yes, indeed. I think we discussed that there isn't any other > alternative as well. Also I was thinking something like in > the 'git log --online' output but that might conflict in some > cases ? (recently there was a progs patch which fixed the short fsid > hash conflict). You mean a shorter UUID? The first four bytes (8 hexa digits) seem unique enough. That can be optinal I think. > > I understand the benefits of automated filtering by FSID, but we have > > conflicting interests here. If you're going to deploy automated log > > scanning, you probably have automated the filesystem mkfs & mount, so > > it's a matter of configuration to get it done and in one place. > > Makes sense. > > Actually automated scripts wasn't in my discussion, I was thinking > of support engineers debugging customer issues using hand written > scripts. I'm not sure how far should we forsee problems that somebody else may theoretically encounter. Providing grounds for easy navigation and searchability in the logs is on our side, but if somebody does not know how to properly look for them, that's not our problem. > Probably we could taken an experimental project, to have these > logs in cvs format, and use external scripts to monitor and > debug. Make something like splunk.com an easy job. As far as the format stays parseable, everybody should be ok. CSV does not sound like a good idea to me though. > >> Just my understanding: > >> For real end users we need to provide everything at the cli output. > >> That is without asking them to refer to dmesg in the cli out put. IMO. > >> (I could be wrong). Troubleshooters are the people looking at dmesg. > >> So finding the FSID can be expected ? > > > > I don't think that looking up device names is making troubleshooting > > significantly harder and never found it to be a problem in my past > > experienes. > > > > Besides, errors from lower layers report the device names, so bad > > sectors or failed writes pop up very quickly in the searches. > > Right. FSID won't provide that advantage. > > > Either way, I'm willing to make it configurable so it addresses all > > usecases. > > > > Another way came to my mind: make it a module parameter, so > > even the mount option or sysfs settings is not needed and the defaults > > are system-wide. > > Yep. This helps. > I was thinking from the angle of support engineers, who would > solve customers issues (it happens at most of the Linux vendors), > They would write/run scripts on their own to understand the problem > from the kernel logs. So now it can be assured that logs format > would remain same per distribution/vendor. I was working in support for a few years and have an experience with digging in the logs. There was a lot of custom scripting and grepping needed as the problem reports were mostly unique. Some tasks were common so one script doing an initial analysis could be reused without modifications, but otherwise it's not like running a script that would do all the work. Anyway, this is getting too abstract, I'd like to address specific problems. I'm fine with adding the configurable logging options. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> You mean a shorter UUID? The first four bytes (8 hexa digits) seem > unique enough. That can be optinal I think. Yes. I said that. But I am also thinking FSID is user changeable (which is good), and so it should be easy to show that this method fails. >>> Another way came to my mind: make it a module parameter, so >>> even the mount option or sysfs settings is not needed and the defaults >>> are system-wide. > I'm fine with adding the configurable logging > options. This should be fine, I can't think of anything better. Thanks, Anand -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index 4e47849d7427..cc8aee26d17b 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2713,7 +2713,7 @@ int open_ctree(struct super_block *sb, * Pass the whole disk block of size BTRFS_SUPER_INFO_SIZE (4k). */ if (btrfs_check_super_csum(bh->b_data)) { - printk(KERN_ERR "BTRFS: superblock checksum mismatch\n"); + btrfs_err(fs_info, "superblock checksum mismatch"); err = -EINVAL; brelse(bh); goto fail_alloc; @@ -2733,7 +2733,7 @@ int open_ctree(struct super_block *sb, ret = btrfs_check_super_valid(fs_info, sb->s_flags & MS_RDONLY); if (ret) { - printk(KERN_ERR "BTRFS: superblock contains fatal errors\n"); + btrfs_err(fs_info, "superblock contains fatal errors"); err = -EINVAL; goto fail_alloc; } @@ -2768,9 +2768,9 @@ int open_ctree(struct super_block *sb, features = btrfs_super_incompat_flags(disk_super) & ~BTRFS_FEATURE_INCOMPAT_SUPP; if (features) { - printk(KERN_ERR "BTRFS: couldn't mount because of " - "unsupported optional features (%Lx).\n", - features); + btrfs_err(fs_info, + "cannot mount because of unsupported optional features (%llx)", + features); err = -EINVAL; goto fail_alloc; } @@ -2781,7 +2781,7 @@ int open_ctree(struct super_block *sb, features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO; if (features & BTRFS_FEATURE_INCOMPAT_SKINNY_METADATA) - printk(KERN_INFO "BTRFS: has skinny extents\n"); + btrfs_info(fs_info, "has skinny extents"); /* * flag our filesystem as having big metadata blocks if @@ -2789,7 +2789,8 @@ int open_ctree(struct super_block *sb, */ if (btrfs_super_nodesize(disk_super) > PAGE_SIZE) { if (!(features & BTRFS_FEATURE_INCOMPAT_BIG_METADATA)) - printk(KERN_INFO "BTRFS: flagging fs with big metadata feature\n"); + btrfs_info(fs_info, + "flagging fs with big metadata feature"); features |= BTRFS_FEATURE_INCOMPAT_BIG_METADATA; } @@ -2805,9 +2806,9 @@ int open_ctree(struct super_block *sb, */ if ((features & BTRFS_FEATURE_INCOMPAT_MIXED_GROUPS) && (sectorsize != nodesize)) { - printk(KERN_ERR "BTRFS: unequal leaf/node/sector sizes " - "are not allowed for mixed block groups on %s\n", - sb->s_id); + btrfs_err(fs_info, +"unequal nodesize/sectorsize (%u != %u) are not allowed for mixed block groups", + nodesize, sectorsize); goto fail_alloc; } @@ -2820,8 +2821,8 @@ int open_ctree(struct super_block *sb, features = btrfs_super_compat_ro_flags(disk_super) & ~BTRFS_FEATURE_COMPAT_RO_SUPP; if (!(sb->s_flags & MS_RDONLY) && features) { - printk(KERN_ERR "BTRFS: couldn't mount RDWR because of " - "unsupported option features (%Lx).\n", + btrfs_err(fs_info, + "cannot mount read-write because of unsupported optional features (%llx)", features); err = -EINVAL; goto fail_alloc; @@ -2850,8 +2851,7 @@ int open_ctree(struct super_block *sb, ret = btrfs_read_sys_array(tree_root); mutex_unlock(&fs_info->chunk_mutex); if (ret) { - printk(KERN_ERR "BTRFS: failed to read the system " - "array on %s\n", sb->s_id); + btrfs_err(fs_info, "failed to read the system array: %d", ret); goto fail_sb_buffer; } @@ -2865,8 +2865,7 @@ int open_ctree(struct super_block *sb, generation); if (IS_ERR(chunk_root->node) || !extent_buffer_uptodate(chunk_root->node)) { - printk(KERN_ERR "BTRFS: failed to read chunk root on %s\n", - sb->s_id); + btrfs_err(fs_info, "failed to read chunk root"); if (!IS_ERR(chunk_root->node)) free_extent_buffer(chunk_root->node); chunk_root->node = NULL; @@ -2880,8 +2879,7 @@ int open_ctree(struct super_block *sb, ret = btrfs_read_chunk_tree(chunk_root); if (ret) { - printk(KERN_ERR "BTRFS: failed to read chunk tree on %s\n", - sb->s_id); + btrfs_err(fs_info, "failed to read chunk tree: %d", ret); goto fail_tree_roots; } @@ -2892,8 +2890,7 @@ int open_ctree(struct super_block *sb, btrfs_close_extra_devices(fs_devices, 0); if (!fs_devices->latest_bdev) { - printk(KERN_ERR "BTRFS: failed to read devices on %s\n", - sb->s_id); + btrfs_err(fs_info, "failed to read devices"); goto fail_tree_roots; } @@ -2905,8 +2902,7 @@ int open_ctree(struct super_block *sb, generation); if (IS_ERR(tree_root->node) || !extent_buffer_uptodate(tree_root->node)) { - printk(KERN_WARNING "BTRFS: failed to read tree root on %s\n", - sb->s_id); + btrfs_warn(fs_info, "failed to read tree root"); if (!IS_ERR(tree_root->node)) free_extent_buffer(tree_root->node); tree_root->node = NULL; @@ -2938,20 +2934,19 @@ int open_ctree(struct super_block *sb, ret = btrfs_recover_balance(fs_info); if (ret) { - printk(KERN_ERR "BTRFS: failed to recover balance\n"); + btrfs_err(fs_info, "failed to recover balance: %d", ret); goto fail_block_groups; } ret = btrfs_init_dev_stats(fs_info); if (ret) { - printk(KERN_ERR "BTRFS: failed to init dev_stats: %d\n", - ret); + btrfs_err(fs_info, "failed to init dev_stats: %d", ret); goto fail_block_groups; } ret = btrfs_init_dev_replace(fs_info); if (ret) { - pr_err("BTRFS: failed to init dev_replace: %d\n", ret); + btrfs_err(fs_info, "failed to init dev_replace: %d", ret); goto fail_block_groups; } @@ -2959,31 +2954,33 @@ int open_ctree(struct super_block *sb, ret = btrfs_sysfs_add_fsid(fs_devices, NULL); if (ret) { - pr_err("BTRFS: failed to init sysfs fsid interface: %d\n", ret); + btrfs_err(fs_info, "failed to init sysfs fsid interface: %d", + ret); goto fail_block_groups; } ret = btrfs_sysfs_add_device(fs_devices); if (ret) { - pr_err("BTRFS: failed to init sysfs device interface: %d\n", ret); + btrfs_err(fs_info, "failed to init sysfs device interface: %d", + ret); goto fail_fsdev_sysfs; } ret = btrfs_sysfs_add_mounted(fs_info); if (ret) { - pr_err("BTRFS: failed to init sysfs interface: %d\n", ret); + btrfs_err(fs_info, "failed to init sysfs interface: %d", ret); goto fail_fsdev_sysfs; } ret = btrfs_init_space_info(fs_info); if (ret) { - printk(KERN_ERR "BTRFS: Failed to initial space info: %d\n", ret); + btrfs_err(fs_info, "failed to initialize space info: %d", ret); goto fail_sysfs; } ret = btrfs_read_block_groups(fs_info->extent_root); if (ret) { - printk(KERN_ERR "BTRFS: Failed to read block groups: %d\n", ret); + btrfs_err(fs_info, "failed to read block groups: %d", ret); goto fail_sysfs; } fs_info->num_tolerated_disk_barrier_failures = @@ -2991,7 +2988,8 @@ int open_ctree(struct super_block *sb, if (fs_info->fs_devices->missing_devices > fs_info->num_tolerated_disk_barrier_failures && !(sb->s_flags & MS_RDONLY)) { - pr_warn("BTRFS: missing devices(%llu) exceeds the limit(%d), writeable mount is not allowed\n", + btrfs_warn(fs_info, +"missing devices (%llu) exceeds the limit (%d), writeable mount is not allowed", fs_info->fs_devices->missing_devices, fs_info->num_tolerated_disk_barrier_failures); goto fail_sysfs; @@ -3011,8 +3009,7 @@ int open_ctree(struct super_block *sb, if (!btrfs_test_opt(tree_root, SSD) && !btrfs_test_opt(tree_root, NOSSD) && !fs_info->fs_devices->rotating) { - printk(KERN_INFO "BTRFS: detected SSD devices, enabling SSD " - "mode\n"); + btrfs_info(fs_info, "detected SSD devices, enabling SSD mode"); btrfs_set_opt(fs_info->mount_opt, SSD); } @@ -3030,8 +3027,9 @@ int open_ctree(struct super_block *sb, 1 : 0, fs_info->check_integrity_print_mask); if (ret) - printk(KERN_WARNING "BTRFS: failed to initialize" - " integrity check module %s\n", sb->s_id); + btrfs_warn(fs_info, + "failed to initialize integrity check module: %d", + ret); } #endif ret = btrfs_read_qgroup_config(fs_info); @@ -3061,8 +3059,8 @@ int open_ctree(struct super_block *sb, ret = btrfs_recover_relocation(tree_root); mutex_unlock(&fs_info->cleaner_mutex); if (ret < 0) { - printk(KERN_WARNING - "BTRFS: failed to recover relocation\n"); + btrfs_warn(fs_info, "failed to recover relocation: %d", + ret); err = -EINVAL; goto fail_qgroup; } @@ -3083,11 +3081,11 @@ int open_ctree(struct super_block *sb, if (btrfs_test_opt(tree_root, FREE_SPACE_TREE) && !btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) { - pr_info("BTRFS: creating free space tree\n"); + btrfs_info(fs_info, "creating free space tree"); ret = btrfs_create_free_space_tree(fs_info); if (ret) { - pr_warn("BTRFS: failed to create free space tree %d\n", - ret); + btrfs_warn(fs_info, + "failed to create free space tree: %d", ret); close_ctree(tree_root); return ret; } @@ -3104,14 +3102,14 @@ int open_ctree(struct super_block *sb, ret = btrfs_resume_balance_async(fs_info); if (ret) { - printk(KERN_WARNING "BTRFS: failed to resume balance\n"); + btrfs_warn(fs_info, "failed to resume balance: %d", ret); close_ctree(tree_root); return ret; } ret = btrfs_resume_dev_replace_async(fs_info); if (ret) { - pr_warn("BTRFS: failed to resume dev_replace\n"); + btrfs_warn(fs_info, "failed to resume device replace: %d", ret); close_ctree(tree_root); return ret; } @@ -3120,33 +3118,33 @@ int open_ctree(struct super_block *sb, if (btrfs_test_opt(tree_root, CLEAR_CACHE) && btrfs_fs_compat_ro(fs_info, FREE_SPACE_TREE)) { - pr_info("BTRFS: clearing free space tree\n"); + btrfs_info(fs_info, "clearing free space tree"); ret = btrfs_clear_free_space_tree(fs_info); if (ret) { - pr_warn("BTRFS: failed to clear free space tree %d\n", - ret); + btrfs_warn(fs_info, + "failed to clear free space tree: %d", ret); close_ctree(tree_root); return ret; } } if (!fs_info->uuid_root) { - pr_info("BTRFS: creating UUID tree\n"); + btrfs_info(fs_info, "creating UUID tree"); ret = btrfs_create_uuid_tree(fs_info); if (ret) { - pr_warn("BTRFS: failed to create the UUID tree %d\n", - ret); + btrfs_warn(fs_info, + "failed to create the UUID tree: %d", ret); close_ctree(tree_root); return ret; } } else if (btrfs_test_opt(tree_root, RESCAN_UUID_TREE) || fs_info->generation != btrfs_super_uuid_tree_generation(disk_super)) { - pr_info("BTRFS: checking UUID tree\n"); + btrfs_info(fs_info, "checking UUID tree"); ret = btrfs_check_uuid_tree(fs_info); if (ret) { - pr_warn("BTRFS: failed to check the UUID tree %d\n", - ret); + btrfs_warn(fs_info, + "failed to check the UUID tree: %d", ret); close_ctree(tree_root); return ret; }
Currently we lack the identification of the filesystem in most if not all mount messages, done via printk/pr_* functions. We can use the btrfs_* helpers in open_ctree, as the fs_info <-> sb link is established at the beginning of the function. The messages have been updated at the same time to be more consistent: * dropped sb->s_id, as it's not available via btrfs_* * added %d for return code where appropriate * wording changed * %Lx replaced by %llx Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/disk-io.c | 102 ++++++++++++++++++++++++++--------------------------- 1 file changed, 50 insertions(+), 52 deletions(-)