Message-ID: <20260116085359.GD15119@lst.de>
Date: Fri, 16 Jan 2026 09:53:59 +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, Hyunchul Lee <hyc.lee@...il.com>
Subject: Re: [PATCH v5 06/14] ntfs: update file operations

On Sun, Jan 11, 2026 at 11:03:36PM +0900, Namjae Jeon wrote:
>  /**
> + * ntfs_setattr - called from notify_change() when an attribute is being changed
> + * @idmap:	idmap of the mount the inode was found from
> + * @dentry:	dentry whose attributes to change
> + * @attr:	structure describing the attributes and the changes
>   *
> + * We have to trap VFS attempts to truncate the file described by @dentry as
> + * soon as possible, because we do not implement changes in i_size yet.  So we
> + * abort all i_size changes here.
>   *
> + * We also abort all changes of user, group, and mode as we do not implement
> + * the NTFS ACLs yet.

This comment isn't actually true, is it?  Also having kerneldoc comments
for something that implements VFS methods isn't generally very useful,
they should have their API documentation in the VFS documentation.  You
can comment anything special in a normal code comment if it applies.

> +	if (ia_valid & ATTR_SIZE) {
> +		if (NInoCompressed(ni) || NInoEncrypted(ni)) {
> +			ntfs_warning(vi->i_sb,
> +				     "Changes in inode size are not supported yet for %s files, ignoring.",
> +				     NInoCompressed(ni) ? "compressed" : "encrypted");
> +			err = -EOPNOTSUPP;

This is still quite a limitation.  But I also think you need a goto
to exit early here instead allowing the other attribute changes to
be applied?

Also experience from other file systems suggests splitting the ATTR_SIZE
handling into a separate helper tends to really help structuring the
code in general.

> +int ntfs_getattr(struct mnt_idmap *idmap, const struct path *path,
> +		struct kstat *stat, unsigned int request_mask,
> +		unsigned int query_flags)
>  {

Can you add support DIO alignment reporting here?  Especially with
things like compressed files this would be very useful.

> +static loff_t ntfs_file_llseek(struct file *file, loff_t offset, int whence)
>  {
> +	struct inode *vi = file->f_mapping->host;
> +
> +	if (whence == SEEK_DATA || whence == SEEK_HOLE) {

I'd stick to the structure of the XFS and ext4 llseek implementation
here and switch on whence and call the fitting helpers as needed.

Talking about helpers, why does iomap_seek_hole/iomap_seek_data
not work for ntfs?

> +		file_accessed(iocb->ki_filp);
> +		ret = iomap_dio_rw(iocb, to, &ntfs_read_iomap_ops, NULL, IOMAP_DIO_PARTIAL,

Why do you need IOMAP_DIO_PARTIAL?  That's mostly a workaround
for "interesting" locking in btrfs and gfs2.  If ntfs has similar
issues, it would be helpful to add a comment here.  Also maybe fix
the overly long line.

> +	if (NInoNonResident(ni) && (iocb->ki_flags & IOCB_DIRECT) &&
> +	    ((iocb->ki_pos | ret) & (vi->i_sb->s_blocksize - 1))) {
> +		ret = -EINVAL;
> +		goto out_lock;
> +	}

iomap_dio_rw now has a IOMAP_DIO_FSBLOCK_ALIGNED to do these
checks.  Also please throw in a comment why ntrfs needs fsblock
alignment.

> +	if (iocb->ki_pos + ret > old_data_size) {
> +		mutex_lock(&ni->mrec_lock);
> +		if (!NInoCompressed(ni) && iocb->ki_pos + ret > ni->allocated_size &&
> +		    iocb->ki_pos + ret < ni->allocated_size + vol->preallocated_size)
> +			ret = ntfs_attr_expand(ni, iocb->ki_pos + ret,
> +					ni->allocated_size + vol->preallocated_size);
> +		else if (NInoCompressed(ni) && iocb->ki_pos + ret > ni->allocated_size)
> +			ret = ntfs_attr_expand(ni, iocb->ki_pos + ret,
> +				round_up(iocb->ki_pos + ret, ni->itype.compressed.block_size));
> +		else
> +			ret = ntfs_attr_expand(ni, iocb->ki_pos + ret, 0);
> +		mutex_unlock(&ni->mrec_lock);
> +		if (ret < 0)
> +			goto out;
> +	}

What is the reason to do the expansion here instead of in the iomap_begin
handler when we know we are committed to write to range?

> +	if (NInoNonResident(ni) && iocb->ki_flags & IOCB_DIRECT) {

Mayube split this direct I/O branch which is quite huge into a separate
helper, similar to what a lof of other file systems are doing?

>  	}
> +out:
> +	if (ret < 0 && ret != -EIOCBQUEUED) {
> +out_err:
> +		if (ni->initialized_size != old_init_size) {
> +			mutex_lock(&ni->mrec_lock);
> +			ntfs_attr_set_initialized_size(ni, old_init_size);
> +			mutex_unlock(&ni->mrec_lock);
> +		}
> +		if (ni->data_size != old_data_size) {
> +			truncate_setsize(vi, old_data_size);
> +			ntfs_attr_truncate(ni, old_data_size);
> +		}

Don't you also need to this in dio I/O completion handler for async
writes?  (actually I guess they aren't supported, I'll try to find the
code for that).

> +static vm_fault_t ntfs_filemap_page_mkwrite(struct vm_fault *vmf)
>  {
> +	vm_fault_t ret;
> +
> +	if (unlikely(IS_IMMUTABLE(inode)))
> +		return VM_FAULT_SIGBUS;

I don't think the VM ever allows write faults on files not opened for
writing, which can't be done for IS_IMMUTABLE files.  If you could ever
hit this we have a huge problem in the upper layers that needs fixing.

> +static int ntfs_ioctl_fitrim(struct ntfs_volume *vol, unsigned long arg)
> +{
> +	struct fstrim_range __user *user_range;
> +	struct fstrim_range range;
> +	struct block_device *dev;
>  	int err;
>  
> +static long ntfs_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
>  {
> +	struct inode *vi = file_inode(file);
> +	struct ntfs_inode *ni = NTFS_I(vi);
> +	struct ntfs_volume *vol = ni->vol;
> +	int err = 0;
> +	loff_t end_offset = offset + len;
> +	loff_t old_size, new_size;
> +	s64 start_vcn, end_vcn;
> +	bool map_locked = false;
> +
> +	if (!S_ISREG(vi->i_mode))
> +		return -EOPNOTSUPP;

ntfs_fallocate is only wired up in ntfs_file_ops, so this can't
happen.

> +	inode_dio_wait(vi);
> +	if (mode & (FALLOC_FL_PUNCH_HOLE | FALLOC_FL_COLLAPSE_RANGE |
> +		    FALLOC_FL_INSERT_RANGE)) {
> +		filemap_invalidate_lock(vi->i_mapping);
> +		map_locked = true;
> +	}
> +
> +	if (mode & FALLOC_FL_INSERT_RANGE) {

This would benefit a lot from being structured like __xfs_file_fallocate,
that is switch on mode & FALLOC_FL_MODE_MASK for the operation, and
then have a helper for each separate operation type.  The current
huge function is pretty unreadable.

Powered by blists - more mailing lists