Message-ID: <20260116082352.GB15119@lst.de>
Date: Fri, 16 Jan 2026 09:23:52 +0100
From: Christoph Hellwig <hch@....de>
To: Namjae Jeon <linkinjeon@...nel.org>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, hch@....de, tytso@....edu,
	willy@...radead.org, jack@...e.cz, djwong@...nel.org,
	josef@...icpanda.com, sandeen@...deen.net, rgoldwyn@...e.com,
	xiang@...nel.org, dsterba@...e.com, pali@...nel.org,
	ebiggers@...nel.org, neil@...wn.name, amir73il@...il.com,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	iamjoonsoo.kim@....com, cheol.lee@....com, jay.sim@....com,
	gunho.lee@....com
Subject: Re: [PATCH v5 02/14] ntfs: update in-memory, on-disk structures
 and headers

On Sun, Jan 11, 2026 at 11:03:32PM +0900, Namjae Jeon wrote:
> This updates in-memory, on-disk structures, headers and documentation.

A little bit of a description of what is updated would be very
useful.  In fact to review all of the patches except for the first
and the last three would probably easier at least as far as the actual
code is concerned (documentation makes sence to be standalone obviously).

Anyway, I'll chime in here with a few random bits, mostly cosmetic:

> +The new ntfs is an implementation that supports write and the current
> +trends(iomap, no buffer-head) based on read-only classic NTFS.
> 
> +The old read-only ntfs code is much cleaner, with extensive comments,
> +offers readability that makes understanding NTFS easier.
> +The target is to provide current trends(iomap, no buffer head, folio),
> +enhanced performance, stable maintenance, utility support including fsck.

All of this makes sense in a commit message, but not really in persistent
documentation, where all of this, including the "new" gets stale very
quickly.  Also please add a whitespace before the opening brace.

> +- Write support:
> +   Implement write support on classic read-only NTFS. Additionally,
> +   integrate delayed allocation to enhance write performance through
> +   multi-cluster allocation and minimized fragmentation of cluster bitmap.

I'd drop the comparisons with classic NTFS, future readers will barely
have any idea what this is about.

> +
> +- Switch to using iomap:
> +   Use iomap for buffered IO writes, reads, direct IO, file extent mapping,
> +   readpages, writepages operations.
> +
> +- Stop using the buffer head:
> +   The use of buffer head in old ntfs and switched to use folio instead.
> +   As a result, CONFIG_BUFFER_HEAD option enable is removed in Kconfig.
> +
> +- Performance Enhancements:
> +  write, file list browsing, mount performance are improved with
> +  the following.

...

> +- Stability improvement:

...

Similarly, all this is commit message information, not really
for persistent documentation in the source tree.

> - * attrib.h - Defines for attribute handling in NTFS Linux kernel driver.
> - *	      Part of the Linux-NTFS project.
> + * Defines for attribute handling in NTFS Linux kernel driver.
> + * Part of the Linux-NTFS project.

Does the Linux-NTFS project still exists, and in what form is this
part of it?  Sorry for the sneaky question, but that statement feels
a bit weird here.

>   *
>   * Set @count bits starting at bit @start_bit in the bitmap described by the
>   * vfs inode @vi to @value, where @value is either 0 or 1.
> - *
> - * Return 0 on success and -errno on error.
>   */

Any reason for dropping these Return documentations?  From a quick
looks the remove statements still seen to be correct with your
entire series applied.

> +	struct runlist runlist;	/*
> +				 * If state has the NI_NonResident bit set,
> +				 * the runlist of the unnamed data attribute
> +				 * (if a file) or of the index allocation
> +				 * attribute (directory) or of the attribute
> +				 * described by the fake inode (if NInoAttr()).
> +				 * If runlist.rl is NULL, the runlist has not
> +				 * been read in yet or has been unmapped. If
> +				 * NI_NonResident is clear, the attribute is
> +				 * resident (file and fake inode) or there is
> +				 * no $I30 index allocation attribute
> +				 * (small directory). In the latter case
> +				 * runlist.rl is always NULL.
> +				 */

Maybe it's just be, but I think if you write this detailed comments
for fields in a structure, move them above so that you get a lot more
screen real estate and make it more readable.  The same applies
to a lot of places in thee series, and also to bit definitions
(i.e. the NI_* bits very close in the patch here).

>  /*
>   * The full structure containing a ntfs_inode and a vfs struct inode. Used for
>   * all real and fake inodes but not for extent inodes which lack the vfs struct
>   * inode.
>   */
> -typedef struct {
> -	ntfs_inode ntfs_inode;
> +struct big_ntfs_inode {
> +	struct ntfs_inode ntfs_inode;
>  	struct inode vfs_inode;		/* The vfs inode structure. */
> -} big_ntfs_inode;
> +};

It seem like big_ntfs_inode is literally only used in the conversion
helpers below.  Are there are a lot of these "extent inode" so that
not having the vfs inode for them is an actual saving?

(Not an action item for getting this merged, just thinking out loud).

>  /**
>   * NTFS_I - return the ntfs inode given a vfs inode
> @@ -223,22 +269,18 @@ typedef struct {
>   *
>   * NTFS_I() returns the ntfs inode associated with the VFS @inode.
>   */
> -static inline ntfs_inode *NTFS_I(struct inode *inode)
> +static inline struct ntfs_inode *NTFS_I(struct inode *inode)
>  {
> -	return (ntfs_inode *)container_of(inode, big_ntfs_inode, vfs_inode);
> +	return (struct ntfs_inode *)container_of(inode, struct big_ntfs_inode, vfs_inode);

Both the old and new version here aren't good.  Instead of the casts
just dereference the ntfs_inode field in the big_inode:

	return container_of(inode, struct ntfs_big_inode, vfs_inode)->ntfs_inode;

> -static inline struct inode *VFS_I(ntfs_inode *ni)
> +static inline struct inode *VFS_I(struct ntfs_inode *ni)
>  {
> -	return &((big_ntfs_inode *)ni)->vfs_inode;
> +	return &((struct big_ntfs_inode *)ni)->vfs_inode;

Same here, please don't cast:

	return container_of(ni, struct ntfs_big_inode, ntfs_inode)->vf_inode;


>  static inline void *__ntfs_malloc(unsigned long size, gfp_t gfp_mask)
>  {
>  	if (likely(size <= PAGE_SIZE)) {
> -		BUG_ON(!size);
> +		if (!size)
> +			return NULL;
>  		/* kmalloc() has per-CPU caches so is faster for now. */
> -		return kmalloc(PAGE_SIZE, gfp_mask & ~__GFP_HIGHMEM);
> +		return kmalloc(PAGE_SIZE, gfp_mask);
>  		/* return (void *)__get_free_page(gfp_mask); */
>  	}
>  	if (likely((size >> PAGE_SHIFT) < totalram_pages()))
> @@ -49,7 +50,7 @@ static inline void *__ntfs_malloc(unsigned long size, gfp_t gfp_mask)
>   */
>  static inline void *ntfs_malloc_nofs(unsigned long size)
>  {
> -	return __ntfs_malloc(size, GFP_NOFS | __GFP_HIGHMEM);
> +	return __ntfs_malloc(size, GFP_NOFS | __GFP_ZERO);
>  }

This whole ntfs_malloc machinery is pretty outdata in many ways.
I think you're better implementing is using kvmalloc and friends,
and using the _nofs scope where needed.

> +static inline void *ntfs_realloc_nofs(void *addr, unsigned long new_size,
> +		unsigned long cpy_size)

... and kvrealloc here.

> +#define NTFS_DEF_PREALLOC_SIZE		(64*1024*1024)
> +
> +#define STANDARD_COMPRESSION_UNIT	4
> +#define MAX_COMPRESSION_CLUSTER_SIZE 4096

Please throw in comments explaining these magic constants.

> +#define UCHAR_T_SIZE_BITS 1

Why not use sizeof(unsigned char) in the one place using it?

> +#define NTFS_B_TO_CLU(vol, b) ((b) >> (vol)->cluster_size_bits)
> +#define NTFS_CLU_TO_B(vol, clu) ((u64)(clu) << (vol)->cluster_size_bits)
> +#define NTFS_B_TO_CLU_OFS(vol, clu) ((u64)(clu) & (vol)->cluster_size_mask)
> +
> +#define NTFS_MFT_NR_TO_CLU(vol, mft_no) (((u64)mft_no << (vol)->mft_record_size_bits) >> \
> +					 (vol)->cluster_size_bits)
> +#define NTFS_MFT_NR_TO_PIDX(vol, mft_no) (mft_no >> (PAGE_SHIFT - \
> +					  (vol)->mft_record_size_bits))
> +#define NTFS_MFT_NR_TO_POFS(vol, mft_no) (((u64)mft_no << (vol)->mft_record_size_bits) & \
> +					  ~PAGE_MASK)

A lot of this is pretty unreadable.  At least break the line after
the macro definition, e.g.:

#define NTFS_MFT_NR_TO_POFS(vol, mft_no) \
	(((u64)mft_no << (vol)->mft_record_size_bits) & \ ~PAGE_MASK)

But inline functions with proper typing would help a lot.  As would
comments explaining what this does given the not very descriptive
names.

> +/*
> + * ntfs-specific ioctl commands
> + */
> +#define NTFS_IOC_SHUTDOWN _IOR('X', 125, __u32)

This isn't really NTFS-specific, but really something originating
in XFS and then copied to half a dozen file systems.  Maybe start
adding it to uapi/linux/fs.h as a start, and then we'll slowly
migrate the other file system over to it?

Powered by blists - more mailing lists