Message-ID: <CAKYAXd_RoJi5HqQV2NPvmkOTrx9AbSbuCmi=BKieENcLVW0FZg@mail.gmail.com>
Date: Sun, 18 Jan 2026 13:56:55 +0900
From: Namjae Jeon <linkinjeon@...nel.org>
To: Christoph Hellwig <hch@....de>
Cc: viro@...iv.linux.org.uk, brauner@...nel.org, 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 Fri, Jan 16, 2026 at 5:54 PM Christoph Hellwig <hch@....de> wrote:
>
> 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.
Right. Those comments were outdated carryovers and no longer reflect
the current implementation.
I will update them.
>
> > +     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?
Right. I missed the early exit there. I will fix it.
>
> 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.
Okay, I will check it.
>
> > +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.
Okay. I will add it.

>
> > +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.
Okay.
>
> Talking about helpers, why does iomap_seek_hole/iomap_seek_data
> not work for ntfs?
Regarding iomap_seek_hole/iomap_seek_data, the default iomap
implementation treats IOMAP_UNWRITTEN extents as holes unless they
have dirty pages in the page cache. However, in ntfs iomap begin, the
region between initialized_size and i_size (EOF) is mapped as
IOMAP_UNWRITTEN. Since NTFS requires any pre-allocated regions before
initialized_size to be physically zeroed, NTFS must treat all
pre-allocated regions as DATA.

>
> > +             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.
Regarding the use of IOMAP_DIO_PARTIAL, I was not aware that it was a
workaround for specific locking issues in some filesystems. I
incorrectly assumed it was a flag to enable partial success when a DIO
request exceeds the actual data length. I will remove this flags and
fix it.
>
> > +     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.
Okay.
>
> > +     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?
We can probably move it to iomap_begin(). Let me check it.
>
> > +     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?
Okay, I will split it.
>
> >       }
> > +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).
I will check it.
>
> > +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.
Okay, I will remove this.
>
> > +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.
Right, I will remove it.
>
> > +     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.
Okay, I will update them.
Thanks for the review!
>

Powered by blists - more mailing lists