Re: [f2fs-dev] [PATCH v4 0/5] Write-placement hints and FDP
On Tue, Sep 03, 2024 at 07:58:46PM GMT, Kanchan Joshi wrote:
> Hi Amir,
>
>
> On 8/26/2024 10:36 PM, Kanchan Joshi wrote:
> > Current write-hint infrastructure supports 6 temperature-based data life
> > hints.
> > The series extends the infrastructure with a new temperature-agnostic
> > placement-type hint. New fcntl codes F_{SET/GET}_RW_HINT_EX allow to
> > send the hint type/value on file. See patch #3 commit description for
> > the details.
> >
> > Overall this creates 128 placement hint values [*] that users can pass.
> > Patch #5 adds the ability to map these new hint values to nvme-specific
> > placement-identifiers.
> > Patch #4 restricts SCSI to use only life hint values.
> > Patch #1 and #2 are simple prep patches.
> >
> > [*] While the user-interface can support more, this limit is due to the
> > in-kernel plumbing consideration of the inode size. Pahole showed 32-bit
> > hole in the inode, but the code had this comment too:
> >
> > /* 32-bit hole reserved for expanding i_fsnotify_mask */
> >
> > Not must, but it will be good to know if a byte (or two) can be used
> > here.
>
> Since having one extra byte will simplify things, I can't help but ask -
> do you still have the plans to use this space (in entirety) within inode?
I just freed up 8 bytes in struct inode with what's currently in -next.
There will be no using up those 8 bytes unless it's for a good reason
and something that is very widely useful.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: fix to don't set SB_RDONLY in f2fs_handle_critical_error()
On Tue, Sep 10, 2024 at 11:07:13AM GMT, Chao Yu wrote:
> syzbot reports a f2fs bug as below:
>
> [ cut here ]
> WARNING: CPU: 1 PID: 58 at kernel/rcu/sync.c:177 rcu_sync_dtor+0xcd/0x180
> kernel/rcu/sync.c:177
> CPU: 1 UID: 0 PID: 58 Comm: kworker/1:2 Not tainted
> 6.10.0-syzkaller-12562-g1722389b0d86 #0
> Workqueue: events destroy_super_work
> RIP: 0010:rcu_sync_dtor+0xcd/0x180 kernel/rcu/sync.c:177
> Call Trace:
> percpu_free_rwsem+0x41/0x80 kernel/locking/percpu-rwsem.c:42
> destroy_super_work+0xec/0x130 fs/super.c:282
> process_one_work kernel/workqueue.c:3231 [inline]
> process_scheduled_works+0xa2c/0x1830 kernel/workqueue.c:3312
> worker_thread+0x86d/0xd40 kernel/workqueue.c:3390
> kthread+0x2f0/0x390 kernel/kthread.c:389
> ret_from_fork+0x4b/0x80 arch/x86/kernel/process.c:147
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244
>
> As Christian Brauner pointed out [1]: the root cause is f2fs sets
> SB_RDONLY flag in internal function, rather than setting the flag
> covered w/ sb->s_umount semaphore via remount procedure, then below
> race condition causes this bug:
>
> - freeze_super()
> - sb_wait_write(sb, SB_FREEZE_WRITE)
> - sb_wait_write(sb, SB_FREEZE_PAGEFAULT)
> - sb_wait_write(sb, SB_FREEZE_FS)
> - f2fs_handle_critical_error
>- sb->s_flags |= SB_RDONLY
> - thaw_super
> - thaw_super_locked
> - sb_rdonly() is true, so it skips
> sb_freeze_unlock(sb, SB_FREEZE_FS)
> - deactivate_locked_super
>
> Since f2fs has almost the same logic as ext4 [2] when handling critical
> error in filesystem if it mounts w/ errors=remount-ro option:
> - set CP_ERROR_FLAG flag which indicates filesystem is stopped
> - record errors to superblock
> - set SB_RDONLY falg
> Once we set CP_ERROR_FLAG flag, all writable interfaces can detect the
> flag and stop any further updates on filesystem. So, it is safe to not
> set SB_RDONLY flag, let's remove the logic and keep in line w/ ext4 [3].
>
> [1]
> https://lore.kernel.org/all/20240729-himbeeren-funknetz-96e62f9c7aee@brauner
> [2] https://lore.kernel.org/all/20240729132721.hxih6ehigadqf7wx@quack3
> [3] https://lore.kernel.org/linux-ext4/20240805201241.27286-1-j...@suse.cz
>
> Fixes: b62e71be2110 ("f2fs: support errors=remount-ro|continue|panic
> mountoption")
> Reported-by: syzbot+20d7e439f76bbbd86...@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/all/b90a8e061e21d...@google.com/
> Cc: Jan Kara
> Cc: Christian Brauner
> Signed-off-by: Chao Yu
> ---
Thanks!
Reviewed-by: Christian Brauner
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fs/writeback: convert wbc_account_cgroup_owner to take a folio
On Thu, 26 Sep 2024 16:01:21 +0200, Pankaj Raghav (Samsung) wrote: > Most of the callers of wbc_account_cgroup_owner() are converting a folio > to page before calling the function. wbc_account_cgroup_owner() is > converting the page back to a folio to call mem_cgroup_css_from_folio(). > > Convert wbc_account_cgroup_owner() to take a folio instead of a page, > and convert all callers to pass a folio directly except f2fs. > > [...] Applied to the vfs.misc branch of the vfs/vfs.git tree. Patches in the vfs.misc branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs.misc [1/1] fs/writeback: convert wbc_account_cgroup_owner to take a folio https://git.kernel.org/vfs/vfs/c/30dac24e14b5 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] (subset) [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Fri, 04 Jul 2025 10:12:29 +0930, Qu Wenruo wrote: > Currently all the filesystems implementing the > super_opearations::shutdown() callback can not afford losing a device. > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the > involved filesystem. > > But it will no longer be the case, with multi-device filesystems like > btrfs and bcachefs the filesystem can handle certain device loss without > shutting down the whole filesystem. > > [...] Moving this into a stable branch that won't change anymore and that you can pull into btrfs/use as base for your patches. --- Applied to the vfs-6.17.super branch of the vfs/vfs.git tree. Patches in the vfs-6.17.super branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.17.super [1/6] fs: enhance and rename shutdown() callback to remove_bdev() https://git.kernel.org/vfs/vfs/c/165fa94de612 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote: > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote: > > > > > > 在 2025/7/8 08:32, Dave Chinner 写道: > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote: > > > > Currently all the filesystems implementing the > > > > super_opearations::shutdown() callback can not afford losing a device. > > > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for the > > > > involved filesystem. > > > > > > > > But it will no longer be the case, with multi-device filesystems like > > > > btrfs and bcachefs the filesystem can handle certain device loss without > > > > shutting down the whole filesystem. > > > > > > > > To allow those multi-device filesystems to be integrated to use > > > > fs_holder_ops: > > > > > > > > - Replace super_opearation::shutdown() with > > > >super_opearations::remove_bdev() > > > >To better describe when the callback is called. > > > > > > This conflates cause with action. > > > > > > The shutdown callout is an action that the filesystem must execute, > > > whilst "remove bdev" is a cause notification that might require an > > > action to be take. > > > > > > Yes, the cause could be someone doing hot-unplug of the block > > > device, but it could also be something going wrong in software > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable > > > corruption or ENOSPC errors. > > > > > > We already have a "cause" notification: blk_holder_ops->mark_dead(). > > > > > > The generic fs action that is taken by this notification is > > > fs_bdev_mark_dead(). That action is to invalidate caches and shut > > > down the filesystem. > > > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead > > > notification. i.e. it needs an action that is different to > > > fs_bdev_mark_dead(). > > > > > > Indeed, this is how bcachefs already handles "single device > > > died" events for multi-device filesystems - see > > > bch2_fs_bdev_mark_dead(). > > > > I do not think it's the correct way to go, especially when there is already > > fs_holder_ops. > > > > We're always going towards a more generic solution, other than letting the > > individual fs to do the same thing slightly differently. > > On second thought -- it's weird that you'd flush the filesystem and > shrink the inode/dentry caches in a "your device went away" handler. > Fancy filesystems like bcachefs and btrfs would likely just shift IO to > a different bdev, right? And there's no good reason to run shrinkers on > either of those fses, right? > > > Yes, the naming is not perfect and mixing cause and action, but the end > > result is still a more generic and less duplicated code base. > > I think dchinner makes a good point that if your filesystem can do > something clever on device removal, it should provide its own block > device holder ops instead of using fs_holder_ops. I don't understand > why you need a "generic" solution for btrfs when it's not going to do > what the others do anyway. I think letting filesystems implement their own holder ops should be avoided if we can. Christoph may chime in here. I have no appettite for exporting stuff like get_bdev_super() unless absolutely necessary. We tried to move all that handling into the VFS to eliminate a slew of deadlocks we detected and fixed. I have no appetite to repeat that cycle. The shutdown method is implemented only by block-based filesystems and arguably shutdown was always a misnomer because it assumed that the filesystem needs to actually shut down when it is called. IOW, we made it so that it is a call to action but that doesn't have to be the case. Calling it ->remove_bdev() is imo the correct thing because it gives block based filesystem the ability to handle device events how they see fit. Once we will have non-block based filesystems that need a method to always shut down the filesystem itself we might have to revisit this design anyway but no one had that use-case yet. > > Awkward naming is often a sign that further thought (or at least > separation of code) is needed. > > As an aside: > 'twould be nice if we could lift the *FS_IOC_SHUTDOWN dispatch out of > everyone's ioctl functions into the VFS, and then move the "I am dead" > state into super_block so that you could actually shut down any > filesystem, not just the seven that currently implement it. That goes back to my earlier point. Fwiw, I think that's valuable work. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Tue, Jul 08, 2025 at 01:20:50PM -0700, Darrick J. Wong wrote:
> On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote:
> > On Mon 07-07-25 17:45:32, Darrick J. Wong wrote:
> > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote:
> > > > 在 2025/7/8 08:32, Dave Chinner 写道:
> > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote:
> > > > > > Currently all the filesystems implementing the
> > > > > > super_opearations::shutdown() callback can not afford losing a
> > > > > > device.
> > > > > >
> > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for
> > > > > > the
> > > > > > involved filesystem.
> > > > > >
> > > > > > But it will no longer be the case, with multi-device filesystems
> > > > > > like
> > > > > > btrfs and bcachefs the filesystem can handle certain device loss
> > > > > > without
> > > > > > shutting down the whole filesystem.
> > > > > >
> > > > > > To allow those multi-device filesystems to be integrated to use
> > > > > > fs_holder_ops:
> > > > > >
> > > > > > - Replace super_opearation::shutdown() with
> > > > > >super_opearations::remove_bdev()
> > > > > >To better describe when the callback is called.
> > > > >
> > > > > This conflates cause with action.
> > > > >
> > > > > The shutdown callout is an action that the filesystem must execute,
> > > > > whilst "remove bdev" is a cause notification that might require an
> > > > > action to be take.
> > > > >
> > > > > Yes, the cause could be someone doing hot-unplug of the block
> > > > > device, but it could also be something going wrong in software
> > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable
> > > > > corruption or ENOSPC errors.
> > > > >
> > > > > We already have a "cause" notification: blk_holder_ops->mark_dead().
> > > > >
> > > > > The generic fs action that is taken by this notification is
> > > > > fs_bdev_mark_dead(). That action is to invalidate caches and shut
> > > > > down the filesystem.
> > > > >
> > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead
> > > > > notification. i.e. it needs an action that is different to
> > > > > fs_bdev_mark_dead().
> > > > >
> > > > > Indeed, this is how bcachefs already handles "single device
> > > > > died" events for multi-device filesystems - see
> > > > > bch2_fs_bdev_mark_dead().
> > > >
> > > > I do not think it's the correct way to go, especially when there is
> > > > already
> > > > fs_holder_ops.
> > > >
> > > > We're always going towards a more generic solution, other than letting
> > > > the
> > > > individual fs to do the same thing slightly differently.
> > >
> > > On second thought -- it's weird that you'd flush the filesystem and
> > > shrink the inode/dentry caches in a "your device went away" handler.
> > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to
> > > a different bdev, right? And there's no good reason to run shrinkers on
> > > either of those fses, right?
> >
> > I agree it is awkward and bcachefs avoids these in case of removal it can
> > handle gracefully AFAICS.
> >
> > > > Yes, the naming is not perfect and mixing cause and action, but the end
> > > > result is still a more generic and less duplicated code base.
> > >
> > > I think dchinner makes a good point that if your filesystem can do
> > > something clever on device removal, it should provide its own block
> > > device holder ops instead of using fs_holder_ops. I don't understand
> > > why you need a "generic" solution for btrfs when it's not going to do
> > > what the others do anyway.
> >
> > Well, I'd also say just go for own fs_holder_ops if it was not for the
> > awkward "get super from bdev" step. As Christian wrote we've encapsulated
> > that in fs/super.c and bdev_super_lock() in particular but the calling
> > conventions for the fs_holder_ops are not very nice (holding
> > bdev_holder_lock, need to release it before grabbing practically anything
> > else) so I'd have much greater peace of mind if this didn't spread too
> > much. Once you call bdev_super_lock() and hold on to sb with s_umount held,
> > things are much more conventional for the fs land so I'd like if this
> > step happened before any fs hook got called. So I prefer something like
> > Qu's proposal of separate sb op for device removal over exporting
> > bdev_super_lock(). Like:
> >
> > static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
> > {
> > struct super_block *sb;
> >
> > sb = bdev_super_lock(bdev, false);
> > if (!sb)
> > return;
> >
> > if (sb->s_op->remove_bdev) {
> > sb->s_op->remove_bdev(sb, bdev, surprise);
> > return;
> > }
>
> It feels odd but I could live with this, particularly since that's the
> direction that brauner is laying down. :)
I want to reiterate that no one is saying "under no circumstances
implement your own holder ops". But if you rely on the VFS
Re: [f2fs-dev] [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Wed, Jul 09, 2025 at 08:59:42AM +1000, Dave Chinner wrote: > On Tue, Jul 08, 2025 at 09:55:14AM +0200, Christian Brauner wrote: > > On Mon, Jul 07, 2025 at 05:45:32PM -0700, Darrick J. Wong wrote: > > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote: > > > > > > > > > > > > 在 2025/7/8 08:32, Dave Chinner 写道: > > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote: > > > > > > Currently all the filesystems implementing the > > > > > > super_opearations::shutdown() callback can not afford losing a > > > > > > device. > > > > > > > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback for > > > > > > the > > > > > > involved filesystem. > > > > > > > > > > > > But it will no longer be the case, with multi-device filesystems > > > > > > like > > > > > > btrfs and bcachefs the filesystem can handle certain device loss > > > > > > without > > > > > > shutting down the whole filesystem. > > > > > > > > > > > > To allow those multi-device filesystems to be integrated to use > > > > > > fs_holder_ops: > > > > > > > > > > > > - Replace super_opearation::shutdown() with > > > > > >super_opearations::remove_bdev() > > > > > >To better describe when the callback is called. > > > > > > > > > > This conflates cause with action. > > > > > > > > > > The shutdown callout is an action that the filesystem must execute, > > > > > whilst "remove bdev" is a cause notification that might require an > > > > > action to be take. > > > > > > > > > > Yes, the cause could be someone doing hot-unplug of the block > > > > > device, but it could also be something going wrong in software > > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable > > > > > corruption or ENOSPC errors. > > > > > > > > > > We already have a "cause" notification: blk_holder_ops->mark_dead(). > > > > > > > > > > The generic fs action that is taken by this notification is > > > > > fs_bdev_mark_dead(). That action is to invalidate caches and shut > > > > > down the filesystem. > > > > > > > > > > btrfs needs to do something different to a blk_holder_ops->mark_dead > > > > > notification. i.e. it needs an action that is different to > > > > > fs_bdev_mark_dead(). > > > > > > > > > > Indeed, this is how bcachefs already handles "single device > > > > > died" events for multi-device filesystems - see > > > > > bch2_fs_bdev_mark_dead(). > > > > > > > > I do not think it's the correct way to go, especially when there is > > > > already > > > > fs_holder_ops. > > > > > > > > We're always going towards a more generic solution, other than letting > > > > the > > > > individual fs to do the same thing slightly differently. > > > > > > On second thought -- it's weird that you'd flush the filesystem and > > > shrink the inode/dentry caches in a "your device went away" handler. > > > Fancy filesystems like bcachefs and btrfs would likely just shift IO to > > > a different bdev, right? And there's no good reason to run shrinkers on > > > either of those fses, right? > > > > > > > Yes, the naming is not perfect and mixing cause and action, but the end > > > > result is still a more generic and less duplicated code base. > > > > > > I think dchinner makes a good point that if your filesystem can do > > > something clever on device removal, it should provide its own block > > > device holder ops instead of using fs_holder_ops. I don't understand > > > why you need a "generic" solution for btrfs when it's not going to do > > > what the others do anyway. > > > > I think letting filesystems implement their own holder ops should be > > avoided if we can. Christoph may chime in here. I have no appettite for > > exporting stuff like get_bdev_super() unless absolutely necessary. We > > tried to move all that handling into the VFS to eliminate a slew of > > deadlocks we detected and fixed. I have no appetite to repeat that > > cycle. > > Except it isn't actually necessary. > > Everyone here seems to be assuming that the filesystem *must* take > an active superblock reference to process a device removal event, > and that is *simply not true*. > > bcachefs does not use get_bdev_super() or an active superblock > reference to process ->mark_dead events. > > It has it's own internal reference counting on the struct bch_fs > attached to the bdev that ensures the filesystem structures can't go > away whilst ->mark_dead is being processed. i.e. bcachefs is only > dependent on the bdev->bd_holder_lock() being held when > ->mark_dead() is called and does not rely on the VFS for anything. > > This means that device removal processing can be performed > without global filesystem/VFS locks needing to be held. Hence issues > like re-entrancy deadlocks when there are concurrent/cascading > device failures (e.g. a HBA dies, taking out multiple devices > simultaneously) are completely avoided... > > It also avoids the problem of ->mark_dead events being generated > from a context that holds files
Re: [f2fs-dev] [PATCH v4 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Thu, Jul 10, 2025 at 07:24:46PM +0930, Qu Wenruo wrote: > > > 在 2025/7/10 18:10, Christian Brauner 写道: > > On Tue, Jul 08, 2025 at 01:20:50PM -0700, Darrick J. Wong wrote: > > > On Tue, Jul 08, 2025 at 12:20:00PM +0200, Jan Kara wrote: > > > > On Mon 07-07-25 17:45:32, Darrick J. Wong wrote: > > > > > On Tue, Jul 08, 2025 at 08:52:47AM +0930, Qu Wenruo wrote: > > > > > > 在 2025/7/8 08:32, Dave Chinner 写道: > > > > > > > On Fri, Jul 04, 2025 at 10:12:29AM +0930, Qu Wenruo wrote: > > > > > > > > Currently all the filesystems implementing the > > > > > > > > super_opearations::shutdown() callback can not afford losing a > > > > > > > > device. > > > > > > > > > > > > > > > > Thus fs_bdev_mark_dead() will just call the shutdown() callback > > > > > > > > for the > > > > > > > > involved filesystem. > > > > > > > > > > > > > > > > But it will no longer be the case, with multi-device > > > > > > > > filesystems like > > > > > > > > btrfs and bcachefs the filesystem can handle certain device > > > > > > > > loss without > > > > > > > > shutting down the whole filesystem. > > > > > > > > > > > > > > > > To allow those multi-device filesystems to be integrated to use > > > > > > > > fs_holder_ops: > > > > > > > > > > > > > > > > - Replace super_opearation::shutdown() with > > > > > > > > super_opearations::remove_bdev() > > > > > > > > To better describe when the callback is called. > > > > > > > > > > > > > > This conflates cause with action. > > > > > > > > > > > > > > The shutdown callout is an action that the filesystem must > > > > > > > execute, > > > > > > > whilst "remove bdev" is a cause notification that might require an > > > > > > > action to be take. > > > > > > > > > > > > > > Yes, the cause could be someone doing hot-unplug of the block > > > > > > > device, but it could also be something going wrong in software > > > > > > > layers below the filesystem. e.g. dm-thinp having an unrecoverable > > > > > > > corruption or ENOSPC errors. > > > > > > > > > > > > > > We already have a "cause" notification: > > > > > > > blk_holder_ops->mark_dead(). > > > > > > > > > > > > > > The generic fs action that is taken by this notification is > > > > > > > fs_bdev_mark_dead(). That action is to invalidate caches and shut > > > > > > > down the filesystem. > > > > > > > > > > > > > > btrfs needs to do something different to a > > > > > > > blk_holder_ops->mark_dead > > > > > > > notification. i.e. it needs an action that is different to > > > > > > > fs_bdev_mark_dead(). > > > > > > > > > > > > > > Indeed, this is how bcachefs already handles "single device > > > > > > > died" events for multi-device filesystems - see > > > > > > > bch2_fs_bdev_mark_dead(). > > > > > > > > > > > > I do not think it's the correct way to go, especially when there is > > > > > > already > > > > > > fs_holder_ops. > > > > > > > > > > > > We're always going towards a more generic solution, other than > > > > > > letting the > > > > > > individual fs to do the same thing slightly differently. > > > > > > > > > > On second thought -- it's weird that you'd flush the filesystem and > > > > > shrink the inode/dentry caches in a "your device went away" handler. > > > > > Fancy filesystems like bcachefs and btrfs would likely just shift IO > > > > > to > > > > > a different bdev, right? And there's no good reason to run shrinkers > > > > > on > > > > > either of those fses, right? > > > > > > > > I agree it is awkward and bcachefs avoids these in case of removal it > > > > can > > > > handle gracefully AFAICS. > > > > > > > > > > Yes, the naming is not perfect and mixing cause and action, but the > > > > > > end > > > > > > result is still a more generic and less duplicated code base. > > > > > > > > > > I think dchinner makes a good point that if your filesystem can do > > > > > something clever on device removal, it should provide its own block > > > > > device holder ops instead of using fs_holder_ops. I don't understand > > > > > why you need a "generic" solution for btrfs when it's not going to do > > > > > what the others do anyway. > > > > > > > > Well, I'd also say just go for own fs_holder_ops if it was not for the > > > > awkward "get super from bdev" step. As Christian wrote we've > > > > encapsulated > > > > that in fs/super.c and bdev_super_lock() in particular but the calling > > > > conventions for the fs_holder_ops are not very nice (holding > > > > bdev_holder_lock, need to release it before grabbing practically > > > > anything > > > > else) so I'd have much greater peace of mind if this didn't spread too > > > > much. Once you call bdev_super_lock() and hold on to sb with s_umount > > > > held, > > > > things are much more conventional for the fs land so I'd like if this > > > > step happened before any fs hook got called. So I prefer something like > > > > Qu's proposal of separate sb op for device removal over exporting > > > > bdev_super_lock(). Like: > > > > > > > > stati
Re: [f2fs-dev] [PATCH v2 1/6] fs: enhance and rename shutdown() callback to remove_bdev()
On Tue, Jul 01, 2025 at 04:05:03PM +0930, Qu Wenruo wrote:
>
>
> 在 2025/7/1 15:44, Christoph Hellwig 写道:
> > On Tue, Jul 01, 2025 at 03:02:34PM +0930, Qu Wenruo wrote:
> > > To allow those multi-device filesystems to be integrated to use
> > > fs_holder_ops:
> > >
> > > - Rename shutdown() call back to remove_bdev()
> > >To better describe when the call back is called.
> >
> > What is renamed back here?
>
> Rename the old shutdown to remove_bdev().
>
> >
> > > -static void exfat_shutdown(struct super_block *sb)
> > > +static void exfat_shutdown(struct super_block *sb, struct block_device
> > > *bdev)
> > > {
> > > exfat_force_shutdown(sb, EXFAT_GOING_DOWN_NOSYNC);
> > > }
> > > @@ -202,7 +202,7 @@ static const struct super_operations exfat_sops = {
> > > .put_super = exfat_put_super,
> > > .statfs = exfat_statfs,
> > > .show_options = exfat_show_options,
> > > - .shutdown = exfat_shutdown,
> > > + .remove_bdev= exfat_shutdown,
> >
> > Please also rename the function so that they match the method name.
>
> I prefer not, and it is intentionally left as is.
>
> This give us a very clear view what a fs is expected to do.
Qu, would you please rename the individual functions?
The NAK later just because of this is unnecessary. I will say clearly
that I will ignore gratuitous NAKs that are premised on large scale
rewrites that are out of scope for the problem.
Here the requested rework has an acceptable scope though and we can
sidestep the whole problem and solve it so everyone's happy.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/10] convert the majority of file systems to mmap_prepare
On Mon, Jun 16, 2025 at 04:11:11PM -0700, Andrew Morton wrote: > On Mon, 16 Jun 2025 20:33:19 +0100 Lorenzo Stoakes > wrote: > > > I am basing this on the mm-new branch in Andrew's tree, so let me know if I > > should rebase anything here. Given the mm bits touched I did think perhaps > > we should take it through the mm tree, however it may be more sensible to > > take it through an fs tree - let me know! > > It's more fs/ than mm/ purely from a footprint point of view. But > there any expectation that there will be additional patches which build > on this? > > I'll scoop it into mm-new for now, see what happens. I'm going to carry this in the vfs-6.17.mmap_prepare branch after fixing up the various minor issues spotted in the series. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 06/10] fs/xfs: transition from deprecated .mmap hook to .mmap_prepare
On Mon, Jun 16, 2025 at 10:08:03PM -0700, Christoph Hellwig wrote: > On Mon, Jun 16, 2025 at 08:33:25PM +0100, Lorenzo Stoakes wrote: > > STATIC int > > -xfs_file_mmap( > > - struct file *file, > > - struct vm_area_struct *vma) > > +xfs_file_mmap_prepare( > > + struct vm_area_desc *desc) > > Please stick to the existing alignment for the declarations. > > Otherwise this looks good. Fixed in-tree. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 03/10] fs: consistently use file_has_valid_mmap_hooks() helper
On Tue, Jun 17, 2025 at 12:08:13PM +0200, Jan Kara wrote:
> On Tue 17-06-25 06:25:34, Lorenzo Stoakes wrote:
> > On Mon, Jun 16, 2025 at 10:11:28PM -0700, Christoph Hellwig wrote:
> > > On Mon, Jun 16, 2025 at 08:33:22PM +0100, Lorenzo Stoakes wrote:
> > > > Since commit c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file
> > > > callback"), the f_op->mmap() hook has been deprecated in favour of
> > > > f_op->mmap_prepare().
> > > >
> > > > Additionally, commit bb666b7c2707 ("mm: add mmap_prepare() compatibility
> > > > layer for nested file systems") permits the use of the .mmap_prepare()
> > > > hook
> > > > even in nested filesystems like overlayfs.
> > > >
> > > > There are a number of places where we check only for f_op->mmap - this
> > > > is
> > > > incorrect now mmap_prepare exists, so update all of these to use the
> > > > general helper file_has_valid_mmap_hooks().
> > > >
> > > > Most notably, this updates the elf logic to allow for the ability to
> > > > execute binaries on filesystems which have the .mmap_prepare hook, but
> > > > additionally we update nested filesystems.
> > >
> > > Can you please give the function a better name before spreading it?
> > > file operations aren't hooks by any classic definition.
> > >
> >
> > can_mmap_file()?
>
> I like this name more as well :). With this patch looks good to me. Again a
Fixed in-tree.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 04/10] fs/dax: make it possible to check dev dax support without a VMA
On Mon, Jun 16, 2025 at 09:26:54PM +0100, Matthew Wilcox wrote:
> On Mon, Jun 16, 2025 at 08:33:23PM +0100, Lorenzo Stoakes wrote:
> > fs/ext4/file.c | 2 +-
> > fs/xfs/xfs_file.c | 3 ++-
>
> Both of these already have the inode from the file ...
>
> > +static inline bool daxdev_mapping_supported(vm_flags_t vm_flags,
> > + struct file *file,
> > + struct dax_device *dax_dev)
> > {
> > - if (!(vma->vm_flags & VM_SYNC))
> > + if (!(vm_flags & VM_SYNC))
> > return true;
> > - if (!IS_DAX(file_inode(vma->vm_file)))
> > + if (!IS_DAX(file_inode(file)))
> > return false;
> > return dax_synchronous(dax_dev);
>
> ... and the only thing this function uses from the file is the inode.
> So maybe pass in the inode rather than the file?
Agreed. I've converted this to take const struct inode *.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 09/10] fs: convert most other generic_file_*mmap() users to .mmap_prepare()
On Tue, Jun 17, 2025 at 12:23:41PM +0200, Jan Kara wrote:
> On Mon 16-06-25 20:33:28, Lorenzo Stoakes wrote:
> > Update nearly all generic_file_mmap() and generic_file_readonly_mmap()
> > callers to use generic_file_mmap_prepare() and
> > generic_file_readonly_mmap_prepare() respectively.
> >
> > We update blkdev, 9p, afs, erofs, ext2, nfs, ntfs3, smb, ubifs and vboxsf
> > file systems this way.
> >
> > Remaining users we cannot yet update are ecryptfs, fuse and cramfs. The
> > former two are nested file systems that must support any underlying file
> > ssytem, and cramfs inserts a mixed mapping which currently requires a VMA.
> >
> > Once all file systems have been converted to mmap_prepare(), we can then
> > update nested file systems.
> >
> > Signed-off-by: Lorenzo Stoakes
>
> Overall the patch looks good. Just a couple of notes regarding pointless
> local variable being created...
>
> > ---
> > block/fops.c | 9 +
> > fs/9p/vfs_file.c | 11 ++-
> > fs/afs/file.c | 11 ++-
> > fs/erofs/data.c| 16 +---
> > fs/ext2/file.c | 12 +++-
> > fs/nfs/file.c | 13 +++--
> > fs/nfs/internal.h | 2 +-
> > fs/nfs/nfs4file.c | 2 +-
> > fs/ntfs3/file.c| 15 ---
> > fs/smb/client/cifsfs.c | 12 ++--
> > fs/smb/client/cifsfs.h | 4 ++--
> > fs/smb/client/file.c | 14 --
> > fs/ubifs/file.c| 8
> > fs/vboxsf/file.c | 8
> > 14 files changed, 74 insertions(+), 63 deletions(-)
> >
> > diff --git a/block/fops.c b/block/fops.c
> > index 1309861d4c2c..5a0ebc81e489 100644
> > --- a/block/fops.c
> > +++ b/block/fops.c
> > @@ -911,14 +911,15 @@ static long blkdev_fallocate(struct file *file, int
> > mode, loff_t start,
> > return error;
> > }
> >
> > -static int blkdev_mmap(struct file *file, struct vm_area_struct *vma)
> > +static int blkdev_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > struct inode *bd_inode = bdev_file_inode(file);
>
> I guess no need to create 'file' variable here since it has only one use in
> the line above...
Agreed, fixed in-tree.
> > -static int afs_file_mmap(struct file *file, struct vm_area_struct *vma)
> > +static int afs_file_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
>
> Same comment about pointless local variable here as well.
Same.
> > -static int erofs_file_mmap(struct file *file, struct vm_area_struct *vma)
> > +static int erofs_file_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > +
> > if (!IS_DAX(file_inode(file)))
>
> And here...
Same.
> > -int cifs_file_strict_mmap(struct file *file, struct vm_area_struct *vma)
> > +int cifs_file_strict_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > int xid, rc = 0;
> > struct inode *inode = file_inode(file);
>
> Again pointless local variable 'file' here.
Same.
> > -int cifs_file_mmap(struct file *file, struct vm_area_struct *vma)
> > +int cifs_file_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > int rc, xid;
>
> And here (the only use is in cifs_revalidate_file(file)).
Same.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 10/10] fs: replace mmap hook with .mmap_prepare for simple mappings
On Tue, Jun 17, 2025 at 12:28:17PM +0200, Jan Kara wrote:
> On Mon 16-06-25 20:33:29, Lorenzo Stoakes wrote:
> > Since commit c84bf6dd2b83 ("mm: introduce new .mmap_prepare() file
> > callback"), the f_op->mmap() hook has been deprecated in favour of
> > f_op->mmap_prepare().
> >
> > This callback is invoked in the mmap() logic far earlier, so error handling
> > can be performed more safely without complicated and bug-prone state
> > unwinding required should an error arise.
> >
> > This hook also avoids passing a pointer to a not-yet-correctly-established
> > VMA avoiding any issues with referencing this data structure.
> >
> > It rather provides a pointer to the new struct vm_area_desc descriptor type
> > which contains all required state and allows easy setting of required
> > parameters without any consideration needing to be paid to locking or
> > reference counts.
> >
> > Note that nested filesystems like overlayfs are compatible with an
> > .mmap_prepare() callback since commit bb666b7c2707 ("mm: add mmap_prepare()
> > compatibility layer for nested file systems").
> >
> > In this patch we apply this change to file systems with relatively simple
> > mmap() hook logic - exfat, ceph, f2fs, bcachefs, zonefs, btrfs, ocfs2,
> > orangefs, nilfs2, romfs, ramfs and aio.
> >
> > Signed-off-by: Lorenzo Stoakes
>
> Two small nits below. Otherwise feel free to add:
>
> Reviewed-by: Jan Kara
>
> > diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> > index 60a621b00c65..37522137c380 100644
> > --- a/fs/ceph/addr.c
> > +++ b/fs/ceph/addr.c
> > @@ -2330,13 +2330,14 @@ static const struct vm_operations_struct ceph_vmops
> > = {
> > .page_mkwrite = ceph_page_mkwrite,
> > };
> >
> > -int ceph_mmap(struct file *file, struct vm_area_struct *vma)
> > +int ceph_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
> > struct address_space *mapping = file->f_mapping;
>
> Pointless local variable here...
Agreed, fixed in-tree.
> > -static int exfat_file_mmap(struct file *file, struct vm_area_struct *vma)
> > +static int exfat_file_mmap_prepare(struct vm_area_desc *desc)
> > {
> > + struct file *file = desc->file;
>
> Missing empty line here.
Fixed in-tree.
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH 00/10] convert the majority of file systems to mmap_prepare
On Mon, 16 Jun 2025 20:33:19 +0100, Lorenzo Stoakes wrote: > REVIEWER'S NOTES > > > I am basing this on the mm-new branch in Andrew's tree, so let me know if I > should rebase anything here. Given the mm bits touched I did think perhaps > we should take it through the mm tree, however it may be more sensible to > take it through an fs tree - let me know! > > [...] This looks good. I fixed up the minor review comments. Looking forward to further cleanups in this area. --- Applied to the vfs-6.17.mmap_prepare branch of the vfs/vfs.git tree. Patches in the vfs-6.17.mmap_prepare branch should appear in linux-next soon. Please report any outstanding bugs that were missed during review in a new review to the original patch series allowing us to drop it. It's encouraged to provide Acked-bys and Reviewed-bys even though the patch has now been applied. If possible patch trailers will be updated. Note that commit hashes shown below are subject to change due to rebase, trailer updates or similar. If in doubt, please check the listed branch. tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git branch: vfs-6.17.mmap_prepare [01/10] mm: rename call_mmap/mmap_prepare to vfs_mmap/mmap_prepare https://git.kernel.org/vfs/vfs/c/20ca475d9860 [02/10] mm/nommu: use file_has_valid_mmap_hooks() helper https://git.kernel.org/vfs/vfs/c/c6900f227f89 [03/10] fs: consistently use file_has_valid_mmap_hooks() helper https://git.kernel.org/vfs/vfs/c/b013ed403197 [04/10] fs/dax: make it possible to check dev dax support without a VMA https://git.kernel.org/vfs/vfs/c/0335f6afd348 [05/10] fs/ext4: transition from deprecated .mmap hook to .mmap_prepare https://git.kernel.org/vfs/vfs/c/8c90ae8fe5e3 [06/10] fs/xfs: transition from deprecated .mmap hook to .mmap_prepare https://git.kernel.org/vfs/vfs/c/6528d29b46d8 [07/10] mm/filemap: introduce generic_file_*_mmap_prepare() helpers https://git.kernel.org/vfs/vfs/c/5b44297bcfa4 [08/10] fs: convert simple use of generic_file_*_mmap() to .mmap_prepare() https://git.kernel.org/vfs/vfs/c/951ea2f4844c [09/10] fs: convert most other generic_file_*mmap() users to .mmap_prepare() https://git.kernel.org/vfs/vfs/c/a5ee9a82981d [10/10] fs: replace mmap hook with .mmap_prepare for simple mappings https://git.kernel.org/vfs/vfs/c/a1e5b36c4034 ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 00/13] Move fscrypt and fsverity info out of struct inode
On Sun, Aug 10, 2025 at 02:03:02AM -0700, Eric Biggers wrote: > On Sun, Aug 10, 2025 at 10:47:32AM +0200, Christian Brauner wrote: > > On Sun, Aug 10, 2025 at 12:56:53AM -0700, Eric Biggers wrote: > > > This is a cleaned-up implementation of moving the i_crypt_info and > > > i_verity_info pointers out of 'struct inode' and into the fs-specific > > > part of the inode, as proposed previously by Christian at > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > The high-level concept is still the same: fs/crypto/ and fs/verity/ > > > locate the pointer by adding an offset to the address of struct inode. > > > The offset is retrieved from fscrypt_operations or fsverity_operations. > > > > > > I've cleaned up a lot of the details, including: > > > - Grouped changes into patches differently > > > - Rewrote commit messages and comments to be clearer > > > - Adjusted code formatting to be consistent with existing code > > > - Removed unneeded #ifdefs > > > - Improved choice and location of VFS_WARN_ON_ONCE() statements > > > - Added missing kerneldoc for ubifs_inode::i_crypt_info > > > - Moved field initialization to init_once functions when they exist > > > - Improved ceph offset calculation and removed unneeded static_asserts > > > - fsverity_get_info() now checks IS_VERITY() instead of v_ops > > > - fscrypt_put_encryption_info() no longer checks IS_ENCRYPTED(), since I > > > no longer think it's actually correct there. > > > - verity_data_blocks() now keeps doing a raw dereference > > > - Dropped fscrypt_set_inode_info() > > > - Renamed some functions > > > - Do offset calculation using int, so we don't rely on unsigned overflow > > > - And more. > > > > > > For v4 and earlier, see > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > I'd like to take this series through the fscrypt tree for 6.18. > > > (fsverity normally has a separate tree, but by choosing just one tree > > > for this, we'll avoid conflicts in some places.) > > > > Woh woh. First, I had a cleaned up version ready for v6.18 so if you > > plan on taking over someone's series and resend then maybe ask the > > author first whether that's ok or not. I haven't seen you do that. You > > just caused duplicated work for no reason. > > Ah, sorry about that. When I started looking at it again yesterday > there turned out to be way too many cleanups and fixes I wanted to make > (beyond the comments I gave earlier), and I hadn't seen activity from > you on it in a while. So I figured it would be easier to just send a > series myself. But I should have asked you first, sorry. So I started working on this pretty much right away. And I had planned on sending it out rather soon but then thought to better wait for -rc1 to be released because I saw you had a bunch of crypto changes in for -rc1 that would've caused merge conflicts. It's no big deal overall but I just don't like that I wasted massaging all that stuff. So next time a heads-up would be nice. Thank you! > > > And second general infrastructure changes that touch multiple fses and > > generic fs infrastructure I very much want to go through VFS trees. > > We'll simply use a shared tree. > > So you'd like to discontinue the fscrypt and fsverity trees? That's > what they are for: general infrastructure shared by multiple > filesystems. Or is this comment just for this series in particular, > presumably because it touches 'struct inode'? My comment just applies this series. I'm not here to take away your trees ofc unless you would like to have them go through the VFS batch. That's something that some people like Amir have started doing. I'll put the series into vfs-6.17.inode and push it out then you can base any additional changes on top of that. I'll not touch it unless you tell me to. Linus knows that VFS trees often have work that is used as the base for other trees so he will merge VFS trees before any of the smaller trees and I always mention this to him. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 00/13] Move fscrypt and fsverity info out of struct inode
On Mon, Aug 11, 2025 at 03:17:01PM +0200, Christian Brauner wrote: > On Sun, Aug 10, 2025 at 02:03:02AM -0700, Eric Biggers wrote: > > On Sun, Aug 10, 2025 at 10:47:32AM +0200, Christian Brauner wrote: > > > On Sun, Aug 10, 2025 at 12:56:53AM -0700, Eric Biggers wrote: > > > > This is a cleaned-up implementation of moving the i_crypt_info and > > > > i_verity_info pointers out of 'struct inode' and into the fs-specific > > > > part of the inode, as proposed previously by Christian at > > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > > > The high-level concept is still the same: fs/crypto/ and fs/verity/ > > > > locate the pointer by adding an offset to the address of struct inode. > > > > The offset is retrieved from fscrypt_operations or fsverity_operations. > > > > > > > > I've cleaned up a lot of the details, including: > > > > - Grouped changes into patches differently > > > > - Rewrote commit messages and comments to be clearer > > > > - Adjusted code formatting to be consistent with existing code > > > > - Removed unneeded #ifdefs > > > > - Improved choice and location of VFS_WARN_ON_ONCE() statements > > > > - Added missing kerneldoc for ubifs_inode::i_crypt_info > > > > - Moved field initialization to init_once functions when they exist > > > > - Improved ceph offset calculation and removed unneeded static_asserts > > > > - fsverity_get_info() now checks IS_VERITY() instead of v_ops > > > > - fscrypt_put_encryption_info() no longer checks IS_ENCRYPTED(), since I > > > > no longer think it's actually correct there. > > > > - verity_data_blocks() now keeps doing a raw dereference > > > > - Dropped fscrypt_set_inode_info() > > > > - Renamed some functions > > > > - Do offset calculation using int, so we don't rely on unsigned overflow > > > > - And more. > > > > > > > > For v4 and earlier, see > > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > > > I'd like to take this series through the fscrypt tree for 6.18. > > > > (fsverity normally has a separate tree, but by choosing just one tree > > > > for this, we'll avoid conflicts in some places.) > > > > > > Woh woh. First, I had a cleaned up version ready for v6.18 so if you > > > plan on taking over someone's series and resend then maybe ask the > > > author first whether that's ok or not. I haven't seen you do that. You > > > just caused duplicated work for no reason. > > > > Ah, sorry about that. When I started looking at it again yesterday > > there turned out to be way too many cleanups and fixes I wanted to make > > (beyond the comments I gave earlier), and I hadn't seen activity from > > you on it in a while. So I figured it would be easier to just send a > > series myself. But I should have asked you first, sorry. > > So I started working on this pretty much right away. And I had planned > on sending it out rather soon but then thought to better wait for -rc1 > to be released because I saw you had a bunch of crypto changes in for > -rc1 that would've caused merge conflicts. It's no big deal overall but > I just don't like that I wasted massaging all that stuff. So next time a > heads-up would be nice. Thank you! I just pulled the series and now I see that you also changed the authorship of every single patch in the series from me to you and put my Co-developed-by in there. I mean I acknowledge that there's changes between the branches and there's some function renaming but it's not to the point where authorship should be changed. And if you think that's necessary than it would be something you would want to talk to me about first. I don't care about the stats but it was always hugely frustrating to me when I started kernel development when senior kernel developers waltzed in and thought they'd just take things over so I try very hard to not do that unless this is agreed upon first. > > > And second general infrastructure changes that touch multiple fses and > > > generic fs infrastructure I very much want to go through VFS trees. > > > We'll simply use a shared tree. > > > > So you'd like to discontinue the fscrypt and fsverity trees? That's > > what they are for: general infrastructure shared by multiple > > filesystems. Or is this comment just for this series in particular, > > presumably because it touches 'struct inode'? > > My comment just applies this series. I'm not here to take away your > trees ofc unless you would like to have them go through the VFS batch. > That's something that some people like Amir have started doing. > > I'll put the series into vfs-6.17.inode and push it out then you can > base any additional changes on top of that. I'll not touch it unless you > tell me to. Linus knows that VFS trees often have work that is used as > the base for other trees so he will merge VFS trees before any of the > smaller trees and I always mention this to him. ___ Li
Re: [f2fs-dev] [PATCH v5 00/13] Move fscrypt and fsverity info out of struct inode
> The fs-specific field solution from this patchset is much more efficient > than the rhashtable: efficient enough that we don't really have to worry > about it, regardless of fscrypt or fsverity. So I think it's a good > middle ground, and I'd like to just do it this way. I agree. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 00/13] Move fscrypt and fsverity info out of struct inode
On Sun, Aug 10, 2025 at 12:56:53AM -0700, Eric Biggers wrote: > This is a cleaned-up implementation of moving the i_crypt_info and > i_verity_info pointers out of 'struct inode' and into the fs-specific > part of the inode, as proposed previously by Christian at > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > The high-level concept is still the same: fs/crypto/ and fs/verity/ > locate the pointer by adding an offset to the address of struct inode. > The offset is retrieved from fscrypt_operations or fsverity_operations. > > I've cleaned up a lot of the details, including: > - Grouped changes into patches differently > - Rewrote commit messages and comments to be clearer > - Adjusted code formatting to be consistent with existing code > - Removed unneeded #ifdefs > - Improved choice and location of VFS_WARN_ON_ONCE() statements > - Added missing kerneldoc for ubifs_inode::i_crypt_info > - Moved field initialization to init_once functions when they exist > - Improved ceph offset calculation and removed unneeded static_asserts > - fsverity_get_info() now checks IS_VERITY() instead of v_ops > - fscrypt_put_encryption_info() no longer checks IS_ENCRYPTED(), since I > no longer think it's actually correct there. > - verity_data_blocks() now keeps doing a raw dereference > - Dropped fscrypt_set_inode_info() > - Renamed some functions > - Do offset calculation using int, so we don't rely on unsigned overflow > - And more. > > For v4 and earlier, see > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > I'd like to take this series through the fscrypt tree for 6.18. > (fsverity normally has a separate tree, but by choosing just one tree > for this, we'll avoid conflicts in some places.) Woh woh. First, I had a cleaned up version ready for v6.18 so if you plan on taking over someone's series and resend then maybe ask the author first whether that's ok or not. I haven't seen you do that. You just caused duplicated work for no reason. And second general infrastructure changes that touch multiple fses and generic fs infrastructure I very much want to go through VFS trees. We'll simply use a shared tree. ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH v5 00/13] Move fscrypt and fsverity info out of struct inode
On Mon, Aug 11, 2025 at 09:39:07AM -0700, Eric Biggers wrote: > On Mon, Aug 11, 2025 at 03:34:35PM +0200, Christian Brauner wrote: > > On Mon, Aug 11, 2025 at 03:17:01PM +0200, Christian Brauner wrote: > > > On Sun, Aug 10, 2025 at 02:03:02AM -0700, Eric Biggers wrote: > > > > On Sun, Aug 10, 2025 at 10:47:32AM +0200, Christian Brauner wrote: > > > > > On Sun, Aug 10, 2025 at 12:56:53AM -0700, Eric Biggers wrote: > > > > > > This is a cleaned-up implementation of moving the i_crypt_info and > > > > > > i_verity_info pointers out of 'struct inode' and into the > > > > > > fs-specific > > > > > > part of the inode, as proposed previously by Christian at > > > > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > > > > > > > The high-level concept is still the same: fs/crypto/ and fs/verity/ > > > > > > locate the pointer by adding an offset to the address of struct > > > > > > inode. > > > > > > The offset is retrieved from fscrypt_operations or > > > > > > fsverity_operations. > > > > > > > > > > > > I've cleaned up a lot of the details, including: > > > > > > - Grouped changes into patches differently > > > > > > - Rewrote commit messages and comments to be clearer > > > > > > - Adjusted code formatting to be consistent with existing code > > > > > > - Removed unneeded #ifdefs > > > > > > - Improved choice and location of VFS_WARN_ON_ONCE() statements > > > > > > - Added missing kerneldoc for ubifs_inode::i_crypt_info > > > > > > - Moved field initialization to init_once functions when they exist > > > > > > - Improved ceph offset calculation and removed unneeded > > > > > > static_asserts > > > > > > - fsverity_get_info() now checks IS_VERITY() instead of v_ops > > > > > > - fscrypt_put_encryption_info() no longer checks IS_ENCRYPTED(), > > > > > > since I > > > > > > no longer think it's actually correct there. > > > > > > - verity_data_blocks() now keeps doing a raw dereference > > > > > > - Dropped fscrypt_set_inode_info() > > > > > > - Renamed some functions > > > > > > - Do offset calculation using int, so we don't rely on unsigned > > > > > > overflow > > > > > > - And more. > > > > > > > > > > > > For v4 and earlier, see > > > > > > https://lore.kernel.org/r/20250723-work-inode-fscrypt-v4-0-c8e11488a...@kernel.org/ > > > > > > > > > > > > I'd like to take this series through the fscrypt tree for 6.18. > > > > > > (fsverity normally has a separate tree, but by choosing just one > > > > > > tree > > > > > > for this, we'll avoid conflicts in some places.) > > > > > > > > > > Woh woh. First, I had a cleaned up version ready for v6.18 so if you > > > > > plan on taking over someone's series and resend then maybe ask the > > > > > author first whether that's ok or not. I haven't seen you do that. You > > > > > just caused duplicated work for no reason. > > > > > > > > Ah, sorry about that. When I started looking at it again yesterday > > > > there turned out to be way too many cleanups and fixes I wanted to make > > > > (beyond the comments I gave earlier), and I hadn't seen activity from > > > > you on it in a while. So I figured it would be easier to just send a > > > > series myself. But I should have asked you first, sorry. > > > > > > So I started working on this pretty much right away. And I had planned > > > on sending it out rather soon but then thought to better wait for -rc1 > > > to be released because I saw you had a bunch of crypto changes in for > > > -rc1 that would've caused merge conflicts. It's no big deal overall but > > > I just don't like that I wasted massaging all that stuff. So next time a > > > heads-up would be nice. Thank you! > > > > I just pulled the series and now I see that you also changed the > > authorship of every single patch in the series from me to you and put my > > Co-developed-by in there. > > > > I mean I acknowledge that there's changes between the branches and > > there's some function renaming but it's not to the point where > > authorship should be changed. And if you think that's necessary than it > > would be something you would want to talk to me about first. > > > > I don't care about the stats but it was always hugely frustrating to me > > when I started kernel development when senior kernel developers waltzed > > in and thought they'd just take things over so I try very hard to not do > > that unless this is agreed upon first. > > If you want to keep the authorship that's fine with me. Make sure > you've checked the diff: the diff between v4 and v5 is larger than the > diff between the base and either version. And as I mentioned, I rewrote > the commit messages and divided some of the changes into commits > differently as well. If the situation was flipped, I wouldn't want to > be kept as the author. But I realize there are different opinions about > this sort of thing, and I'm totally fine with whatever you prefer. It's not that I oppose it per se it's just that it would be n
