RE: [PATCH v2 02/10] fs/ntfs3: Add initialization of super block

[Konstantin Komarov]

From: Joe Perches <joe@xxxxxxxxxxx>
Sent: Friday, August 21, 2020 10:39 PM
> 
> On Fri, 2020-08-21 at 16:25 +0000, Konstantin Komarov wrote:
> > Initialization of super block for fs/ntfs3
> []
> > diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c
> []
> > +
> > +/**
> > + * ntfs_trace() - print preformated ntfs specific messages.
> > + */
> > +void __ntfs_trace(const struct super_block *sb, const char *level,
> > +		  const char *fmt, ...)
> 
> This is a printk mechanism.
> 
> I suggest renaming this __ntfs_trace function to ntfs_printk
> as there is a naming expectation conflict with the tracing
> subsystem.
> 
> > +{
[]
> > +	else
> > +		printk("%sntfs3: %s: %pV", level, sb->s_id, &vaf);
> > +	va_end(args);
> > +}
> 
> Also it would be rather smaller overall object code to
> change the macros and uses to embed the KERN_<LEVEL> into
> the format and remove the const char *level argument.
> 
> Use printk_get_level to retrieve the level from the format.
> 
> see fs/f2fs/super.c for an example.
> 
> This could be something like the below with a '\n' addition
> to the format string to ensure that messages are properly
> terminated and cannot be interleaved by other subsystems
> content that might be in another simultaneously running
> thread starting with KERN_CONT.
> 
> void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
> {
> 	struct va_format vaf;
> 	va_list args;
> 	int level;
> 
> 	va_start(args, fmt);
> 
> 	level = printk_get_level(fmt);
> 	vaf.fmt = printk_skip_level(fmt);
> 	vaf.va = &args;
> 	if (!sb)
> 		printk("%c%cntfs3: %pV\n",
> 		       KERN_SOH_ASCII, level, &vaf);
> 	else
> 		printk("%c%cntfs3: %s: %pV\n",
> 		       KERN_SOH_ASCII, level, sbi->sb->s_id, &vaf);
> 
> 	va_end(args);
> }
> 
> > +
> > +/* prints info about inode using dentry case if */
> > +void __ntfs_inode_trace(struct inode *inode, const char *level, const char *fmt,
> 
> ntfs_inode_printk
> 
> > +			...)
> > +{
> > +	struct super_block *sb = inode->i_sb;
> > +	ntfs_sb_info *sbi = sb->s_fs_info;
> > +	struct dentry *dentry;
> > +	const char *name = "?";
> > +	char buf[48];
> > +	va_list args;
> > +	struct va_format vaf;
> > +
> > +	if (!__ratelimit(&sbi->ratelimit))
> > +		return;
> > +
> > +	dentry = d_find_alias(inode);
> > +	if (dentry) {
> > +		spin_lock(&dentry->d_lock);
> > +		name = (const char *)dentry->d_name.name;
> > +	} else {
> > +		snprintf(buf, sizeof(buf), "r=%lx", inode->i_ino);
> > +		name = buf;
> > +	}
> > +
> > +	va_start(args, fmt);
> > +	vaf.fmt = fmt;
> > +	vaf.va = &args;
> > +	printk("%s%s on %s: %pV", level, name, sb->s_id, &vaf);
> > +	va_end(args);
> > +
> > +	if (dentry) {
> > +		spin_unlock(&dentry->d_lock);
> > +		dput(dentry);
> > +	}
> > +}
> 
> Remove level and use printk_get_level as above.
> Format string should use '\n' termination here too.
> 

Thanks for pointing this out and for your effort with the patch, Joe. We will rework logging in V3 so that it's more compliant with Kernel's approach.