| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAKYAXd9PMncz+dK+AJ6aLfMiQ6-xHMuT=zG8qXtE_JOjbtKcig@mail.gmail.com>
Date: Tue, 3 Feb 2026 23:05:42 +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 v6 09/16] ntfs: update iomap and address space operations
On Tue, Feb 3, 2026 at 3:20 PM Christoph Hellwig <hch@....de> wrote:
>
> Suggested commit message:
>
> Update the address space operations to use the iomap framework,
> replacing legacy buffer-head based code.
Okay, I will use it in the next version.
>
> > +#include <linux/mpage.h>
>
> This include should not be needed (same in iomap.c).
Okay.
>
> > +#include <linux/uio.h>
>
> This should not be needed either (same in iomap.c).
Okay.
>
> > -};
> > +static void ntfs_readahead(struct readahead_control *rac)
> > +{
> > + struct address_space *mapping = rac->mapping;
> > + struct inode *inode = mapping->host;
> > + struct ntfs_inode *ni = NTFS_I(inode);
> >
> > + if (!NInoNonResident(ni) || NInoCompressed(ni)) {
> > + /* No readahead for resident and compressed. */
>
> As-is this comment is useless ads it states the obvious. If you
> want to make it useful add why it is not implemented.
Okay, I will improve this comment.
>
> > +static int ntfs_writepages(struct address_space *mapping,
> > + struct writeback_control *wbc)
> > +{
> > + struct inode *inode = mapping->host;
> > + struct ntfs_inode *ni = NTFS_I(inode);
> > + struct iomap_writepage_ctx wpc = {
> > + .inode = mapping->host,
> > + .wbc = wbc,
> > + .ops = &ntfs_writeback_ops,
> > + };
> > +
> > + if (NVolShutdown(ni->vol))
> > + return -EIO;
> >
> > + if (!NInoNonResident(ni))
> > + return 0;
> >
> > + /* If file is encrypted, deny access, just like NT4. */
>
> I don't understand this comment.
Okay, I will update it.
>
> > +void mark_ntfs_record_dirty(struct folio *folio)
> > +{
> > + iomap_dirty_folio(folio->mapping, folio);
> > +}
>
> Should this be in mft.c and have a mft_ compoenent in the name?
Okay, I will move it with renaming.
>
> > + if (!NInoNonResident(ni))
> > + goto out;
>
> Split the resident handling into a helper to keep the method
> simple?
Okay.
>
> > +static int __ntfs_read_iomap_begin(struct inode *inode, loff_t offset, loff_t length,
> > + unsigned int flags, struct iomap *iomap, struct iomap *srcmap,
> > + bool need_unwritten)
> > +{
> > + struct ntfs_inode *ni = NTFS_I(inode);
> > + int ret;
> > +
> > + if (NInoNonResident(ni))
> > + ret = ntfs_read_iomap_begin_non_resident(inode, offset, length,
> > + flags, iomap, need_unwritten);
> > + else
> > + ret = ntfs_read_iomap_begin_resident(inode, offset, length,
> > + flags, iomap);
> > +
> > + return ret;
>
> This could be simplified to:
>
> if (NInoNonResident(NTFS_I(inode)))
> return ntfs_read_iomap_begin_non_resident(inode, offset, length,
> flags, iomap, need_unwritten);
> return ntfs_read_iomap_begin_resident(inode, offset, length, flags,
> iomap);
Okay, I will update it like this.
>
> > +int ntfs_zero_range(struct inode *inode, loff_t offset, loff_t length, bool bdirect)
> > +{
> > + if (bdirect) {
> > + if ((offset | length) & (SECTOR_SIZE - 1))
> > + return -EINVAL;
> > +
> > + return blkdev_issue_zeroout(inode->i_sb->s_bdev,
> > + offset >> SECTOR_SHIFT,
> > + length >> SECTOR_SHIFT,
> > + GFP_NOFS,
> > + BLKDEV_ZERO_NOUNMAP);
> > + }
> > +
> > + return iomap_zero_range(inode,
> > + offset, length,
> > + NULL,
> > + &ntfs_zero_read_iomap_ops,
> > + &ntfs_zero_iomap_folio_ops,
> > + NULL);
> > +}
>
> This really should be two separate helpers.
Okay.
>
> > + if (NInoNonResident(ni)) {
>
> As separate non-resident helper would be useful here.
Okay.
>
> > + if (ntfs_iomap_flags & NTFS_IOMAP_FLAGS_BEGIN)
> > + ret = ntfs_write_iomap_begin_non_resident(inode, offset,
> > + length, iomap);
> > + else
> > + ret = ntfs_write_da_iomap_begin_non_resident(inode,
> > + offset, length, flags, iomap,
> > + ntfs_iomap_flags);
> > + } else {
> > + mutex_lock(&ni->mrec_lock);
> > + ret = ntfs_write_iomap_begin_resident(inode, offset, iomap);
> > + }
> > +
> > + return ret;
>
> But eveven without that, direct returns would really help here:
>
> if (!NInoNonResident(ni)) {
> mutex_lock(&ni->mrec_lock);
> return ntfs_write_iomap_begin_resident(inode, offset, iomap);
> }
>
> ...
>
> if (ntfs_iomap_flags & NTFS_IOMAP_FLAGS_BEGIN)
> return ntfs_write_iomap_begin_non_resident(inode, offset,
> length, iomap);
> return ntfs_write_da_iomap_begin_non_resident(inode, offset, length,
> flags, iomap,
Okay, I will update it like this.
>
> > +static int ntfs_write_iomap_end(struct inode *inode, loff_t pos, loff_t length,
> > + ssize_t written, unsigned int flags, struct iomap *iomap)
> > +{
> > + if (iomap->type == IOMAP_INLINE) {
>
> A separate inline helper would help here as well.
Okay, I will do that.
Thanks for the review!
>
Powered by blists - more mailing lists