From: Kari Argillander <redacted>
Sent: Sunday, January 3, 2021 10:50 PM

To: Konstantin Komarov <almaz.alexandrovich@paragon-software.com>
Cc: linux-fsdevel@vger.kernel.org; viro@zeniv.linux.org.uk; linux-kernel@vger.kernel.org; pali@kernel.org; dsterba@suse.cz;
aaptel@suse.com; willy@infradead.org; rdunlap@infradead.org; joe@perches.com; mark@harmstone.com; nborisov@suse.com;
linux-ntfs-dev@lists.sourceforge.net; anton@tuxera.com; dan.carpenter@oracle.com; hch@lst.de; ebiggers@kernel.org;
andy.lavr@gmail.com
Subject: Re: [PATCH v17 02/10] fs/ntfs3: Add initialization of super block

On Thu, Dec 31, 2020 at 06:23:53PM +0300, Konstantin Komarov wrote:

quoted

diff --git a/fs/ntfs3/fsntfs.c b/fs/ntfs3/fsntfs.c

quoted

+int ntfs_loadlog_and_replay(struct ntfs_inode *ni, struct ntfs_sb_info *sbi)
+{
+	int err = 0;
+	struct super_block *sb = sbi->sb;
+	struct inode *inode = &ni->vfs_inode;
+	struct MFT_REF ref;
+
+	/* Check for 4GB */
+	if (inode->i_size >= 0x100000000ull) {
+		ntfs_err(sb, "$LogFile is too big");
+		err = -EINVAL;
+		goto out;
+	}
+
+	sbi->flags |= NTFS_FLAGS_LOG_REPLAYING;
+
+	ref.low = cpu_to_le32(MFT_REC_MFT);
+	ref.high = 0;
+	ref.seq = cpu_to_le16(1);
+
+	inode = ntfs_iget5(sb, &ref, NULL);
+
+	if (IS_ERR(inode))
+		inode = NULL;
+
+	if (!inode) {
+		/* Try to use mft copy */
+		u64 t64 = sbi->mft.lbo;
+
+		sbi->mft.lbo = sbi->mft.lbo2;
+		inode = ntfs_iget5(sb, &ref, NULL);
+		sbi->mft.lbo = t64;
+		if (IS_ERR(inode))
+			inode = NULL;
+	}
+
+	if (!inode) {
+		err = -EINVAL;
+		ntfs_err(sb, "Failed to load $MFT.");
+		goto out;
+	}
+
+	sbi->mft.ni = ntfs_i(inode);
+
+	err = ni_load_all_mi(sbi->mft.ni);
+	if (!err)
+		err = log_replay(ni);

We only get error from log_replay if

quoted

+
+	iput(inode);
+	sbi->mft.ni = NULL;
+
+	sync_blockdev(sb->s_bdev);
+	invalidate_bdev(sb->s_bdev);
+
+	/* reinit MFT */
+	if (sbi->flags & NTFS_FLAGS_NEED_REPLAY) {
+		err = 0;
+		goto out;
+	}
+
+	if (sb_rdonly(sb))
+		goto out;

we get here. Is this a intentional? Probably but I'm just checking.

Hi Kari! Thanks for your attention on our patches.
This may be indeed quite entangled, here are the cases needed to be
covered:
1) !err && !(sbi->flags & NTFS_FLAGS_NEED_REPLAY) - ok
2) err && !(sbi->flags & NTFS_FLAGS_NEED_REPLAY) - no memory,
  io error, etc on prepare to replay stage
3) !err && (sbi->flags & NTFS_FLAGS_NEED_REPLAY) -
  journal is not empty, storage is readonly or unsupported log version
4) err && (sbi->flags & NTFS_FLAGS_NEED_REPLAY) - no memory, io error,
  etc while replaying
Distinction is that, cases 2/3 lead to mount error every time, while
case 4 permits read-only mount.

quoted

diff --git a/fs/ntfs3/inode.c b/fs/ntfs3/inode.c

quoted

+int ntfs_create_inode(struct inode *dir, struct dentry *dentry,
+		      const struct cpu_str *uni, umode_t mode, dev_t dev,
+		      const char *symname, u32 size, int excl,
+		      struct ntfs_fnd *fnd, struct inode **new_inode)
+{

quoted

+#ifdef CONFIG_NTFS3_FS_POSIX_ACL

In Kconfig this is NTFS3_POSIX_ACL. This repeat every file.

This is OK. You may refer to similar parts of ext4/btrfs sources as a
reference:
fs/ext4/Kconfig or fs/btrfs/Kconfig

quoted

+int ntfs_unlink_inode(struct inode *dir, const struct dentry *dentry)
+{
+	int err;
+	struct super_block *sb = dir->i_sb;
+	struct ntfs_sb_info *sbi = sb->s_fs_info;
+	struct inode *inode = d_inode(dentry);
+	struct ntfs_inode *ni = ntfs_i(inode);
+	const struct qstr *name = &dentry->d_name;
+	struct ntfs_inode *dir_ni = ntfs_i(dir);
+	struct ntfs_index *indx = &dir_ni->dir;
+	struct cpu_str *uni = NULL;
+	struct ATTR_FILE_NAME *fname;
+	u8 name_type;
+	struct ATTR_LIST_ENTRY *le;
+	struct MFT_REF ref;
+	bool is_dir = S_ISDIR(inode->i_mode);
+	struct INDEX_ROOT *dir_root;
+
+	dir_root = indx_get_root(indx, dir_ni, NULL, NULL);
+	if (!dir_root)
+		return -EINVAL;
+
+	ni_lock(ni);
+
+	if (is_dir && !dir_is_empty(inode)) {
+		err = -ENOTEMPTY;
+		goto out1;
+	}
+
+	if (ntfs_is_meta_file(sbi, inode->i_ino)) {
+		err = -EINVAL;
+		goto out1;
+	}
+
+	/* allocate PATH_MAX bytes */
+	uni = __getname();
+	if (!uni) {
+		err = -ENOMEM;
+		goto out1;
+	}
+
+	/* Convert input string to unicode */
+	err = ntfs_nls_to_utf16(sbi, name->name, name->len, uni, NTFS_NAME_LEN,
+				UTF16_HOST_ENDIAN);
+	if (err < 0)
+		goto out4;
+
+	le = NULL;

Little bit random place for this. Do we even need to NULL le.

Thanks. Inititialization is moved to to the place where ni_fname_name
is being called.

quoted

+
+	/*mark rw ntfs as dirty. it will be cleared at umount*/
+	ntfs_set_state(sbi, NTFS_DIRTY_DIRTY);
+
+	/* find name in record */
+#ifdef NTFS3_64BIT_CLUSTER
+	ref.low = cpu_to_le32(dir->i_ino & 0xffffffff);
+	ref.high = cpu_to_le16(dir->i_ino >> 32);
+#else
+	ref.low = cpu_to_le32(dir->i_ino & 0xffffffff);
+	ref.high = 0;
+#endif
+	ref.seq = dir_ni->mi.mrec->seq;
+
+	fname = ni_fname_name(ni, uni, &ref, &le);

quoted

diff --git a/fs/ntfs3/super.c b/fs/ntfs3/super.c

quoted

+#ifdef CONFIG_PRINTK
+/*
+ * Trace warnings/notices/errors
+ * Thanks Joe Perches [off-list ref] for implementation
+ */
+void ntfs_printk(const struct super_block *sb, const char *fmt, ...)
+{
+	struct va_format vaf;
+	va_list args;
+	int level;
+	struct ntfs_sb_info *sbi = sb->s_fs_info;
+
+	/*should we use different ratelimits for warnings/notices/errors? */
+	if (!___ratelimit(&sbi->msg_ratelimit, "ntfs3"))
+		return;
+
+	va_start(args, fmt);
+
+	level = printk_get_level(fmt);
+	vaf.fmt = printk_skip_level(fmt);
+	vaf.va = &args;
+	printk("%c%cntfs3: %s: %pV\n", KERN_SOH_ASCII, level, sb->s_id, &vaf);
+
+	va_end(args);
+}
+
+static char s_name_buf[512];
+static atomic_t s_name_buf_cnt = ATOMIC_INIT(1); // 1 means 'free s_name_buf'
+
+/* print warnings/notices/errors about inode using name or inode number */
+void ntfs_inode_printk(struct inode *inode, const char *fmt, ...)
+{
+	struct super_block *sb = inode->i_sb;
+	struct ntfs_sb_info *sbi = sb->s_fs_info;
+	char *name;
+	va_list args;
+	struct va_format vaf;
+	int level;
+
+	if (!___ratelimit(&sbi->msg_ratelimit, "ntfs3"))
+		return;
+
+	if (atomic_dec_and_test(&s_name_buf_cnt)) {
+		/* use static allocated buffer */
+		name = s_name_buf;
+	} else {
+		name = kmalloc(sizeof(s_name_buf), GFP_NOFS);
+	}
+
+	if (name) {
+		struct dentry *dentry = d_find_alias(inode);
+		const u32 name_len = ARRAY_SIZE(s_name_buf) - 1;
+
+		if (dentry) {
+			spin_lock(&dentry->d_lock);
+			snprintf(name, name_len, "%s", dentry->d_name.name);
+			spin_unlock(&dentry->d_lock);
+			dput(dentry);
+			name[name_len] = 0; /* to be sure*/
+		} else {
+			name[0] = 0;
+		}
+	}
+
+	va_start(args, fmt);
+
+	level = printk_get_level(fmt);
+	vaf.fmt = printk_skip_level(fmt);
+	vaf.va = &args;
+
+	printk("%c%cntfs3: %s: ino=%lx, \"%s\" %pV\n", KERN_SOH_ASCII, level,
+	       sb->s_id, inode->i_ino, name ? name : "", &vaf);
+
+	va_end(args);
+
+	atomic_inc(&s_name_buf_cnt);
+	if (name != s_name_buf)
+		kfree(name);
+}
+#endif

Should these be in debug.c or something? Atleast I do not see point why
they are in super.c.

Overall, the problem file name may be omitted, but it seems to be useful for
debug purposes. This code is placed into super.c because ntfs_printk is described there.

quoted

+static int __init init_ntfs_fs(void)
+{
+	int err;
+
+#ifdef NTFS3_INDEX_BINARY_SEARCH
+	pr_notice("ntfs3: +index binary search\n");
+#endif
+
+#ifdef NTFS3_CHECK_FREE_CLST
+	pr_notice("ntfs3: +check free clusters\n");
+#endif
+
+#if NTFS_LINK_MAX < 0xffff
+	pr_notice("ntfs3: max link count %u\n", NTFS_LINK_MAX);
+#endif
+
+#ifdef NTFS3_64BIT_CLUSTER
+	pr_notice("ntfs3: 64 bits per cluster\n");
+#else
+	pr_notice("ntfs3: 32 bits per cluster\n");
+#endif
+#ifdef CONFIG_NTFS3_LZX_XPRESS
+	pr_notice("ntfs3: read-only lzx/xpress compression included\n");
+#endif
+
+	ntfs_inode_cachep = kmem_cache_create(
+		"ntfs_inode_cache", sizeof(struct ntfs_inode), 0,
+		(SLAB_RECLAIM_ACCOUNT | SLAB_MEM_SPREAD | SLAB_ACCOUNT),
+		init_once);
+	if (!ntfs_inode_cachep) {
+		err = -ENOMEM;
+		goto failed;
+	}
+
+	err = register_filesystem(&ntfs_fs_type);
+	if (!err)
+		return 0;

Do we need kmem_cache_destroy() here if err?

Thanks, this will be fixed in v18.

quoted

+
+failed:
+	return err;
+}