Hi Linus,
Happy new year!
Could you please consider this pull request?
Thank you.
The following changes since commit 6bc40e44f1ddef16a787f3501b97f1fff909177c:
Merge tag 'ovl-fixes-6.7-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/overlayfs/vfs (2023-11-17 09:18:48 -0500)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
for you to fetch changes up to c3c2d45b9050180974e35ec8672c6e788adc236a:
f2fs: show more discard status by sysfs (2023-12-26 13:07:26 -0800)
----------------------------------------------------------------
f2fs update for 6.8-rc1
In this series, we've some progress to support Zoned block device regarding to
the power-cut recovery flow and enabling checkpoint=disable feature which is
essential for Android OTA. Other than that, some patches touched sysfs entries
and tracepoints which are minor, while several bug fixes on error handlers and
compression flows are good to improve the overall stability.
Enhancement:
- enable checkpoint=disable for zoned block device
- sysfs entries such as discard status, discard_io_aware, dir_level
- tracepoints such as f2fs_vm_page_mkwrite(), f2fs_rename(), f2fs_new_inode()
- use shared inode lock during f2fs_fiemap() and f2fs_seek_block()
Bug fix:
- address some power-cut recovery issues on zoned block device
- handle errors and logics on do_garbage_collect(), f2fs_reserve_new_block(),
f2fs_move_file_range(), f2fs_recover_xattr_data()
- don't set FI_PREALLOCATED_ALL for partial write
- fix to update iostat correctly in f2fs_filemap_fault()
- fix to wait on block writeback for post_read case
- fix to tag gcing flag on page during block migration
- restrict max filesize for 16K f2fs
- fix to avoid dirent corruption
- explicitly null-terminate the xattr list
There are also several clean-up patches to remove dead codes and better
readability.
----------------------------------------------------------------
Chao Yu (18):
f2fs: clean up w/ dotdot_name
f2fs: use shared inode lock during f2fs_fiemap()
f2fs: fix to check return value of f2fs_reserve_new_block()
f2fs: fix to avoid dirent corruption
f2fs: introduce tracepoint for f2fs_rename()
f2fs: show i_mode in trace_f2fs_new_inode()
f2fs: sysfs: support discard_io_aware
f2fs: delete obsolete FI_FIRST_BLOCK_WRITTEN
f2fs: delete obsolete FI_DROP_CACHE
f2fs: introduce get_dnode_addr() to clean up codes
f2fs: update blkaddr in __set_data_blkaddr() for cleanup
f2fs: introduce f2fs_invalidate_internal_cache() for cleanup
f2fs: add tracepoint for f2fs_vm_page_mkwrite()
f2fs: fix to tag gcing flag on page during block migration
f2fs: fix to wait on block writeback for post_read case
f2fs: fix to check compress file in f2fs_move_file_range()
f2fs: fix to update iostat correctly in f2fs_filemap_fault()
f2fs: don't set FI_PREALLOCATED_ALL for partial write
Daniel Rosenberg (1):
f2fs: Restrict max filesize for 16K f2fs
Eric Biggers (1):
f2fs: explicitly null-terminate the xattr list
Jaegeuk Kim (6):
f2fs: skip adding a discard command if exists
f2fs: allow checkpoint=disable for zoned block device
f2fs: allocate new section if it's not new
f2fs: fix write pointers on zoned device after roll forward
f2fs: check write pointers when checkpoint=disable
f2fs: let's finish or reset zones all the time
Kevin Hao (1):
f2fs: Use wait_event_freezable_timeout() for freezable kthread
Yang Hubin (1):
f2fs: the name of a struct is wrong in a comment.
Yongpeng Yang (2):
f2fs: Constrain the modification range of dir_level in the sysfs
f2fs: Add error handling for negative returns from do_garbage_collect
Zhiguo Niu (2):
f2fs: fix to check return value of f2fs_recover_xattr_data
f2fs: show more discard status by sysfs
zhangxirui (1):
f2fs: use inode_lock_shared instead of inode_lock in f2fs_seek_block()
Documentation/ABI/testing/sysfs-fs-f2fs | 21 +++++
fs/f2fs/compress.c | 6 +-
fs/f2fs/data.c | 48 ++++-------
fs/f2fs/f2fs.h | 46 +++++++----
fs/f2fs/file.c | 66 +++++++--------
fs/f2fs/gc.c | 16 ++--
fs/f2fs/inode.c | 57 ++++---------
fs/f2fs/namei.c | 23 +++---
fs/f2fs/node.c | 6 +-
fs/f2fs/recovery.c | 25 ++++--
fs/f2fs/segment.c | 138 +++++++++++---------------------
fs/f2fs/super.c | 16 ++--
fs/f2fs/sysfs.c | 50 ++++++++++++
fs/f2fs/xattr.c | 17 +++-
include/linux/f2fs_fs.h | 2 +-
include/trace/events/f2fs.h | 127 +++++++++++++++++++++++++----
16 files changed, 395 insertions(+), 269 deletions(-)
On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <[email protected]> wrote:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
Hmm. I got a somewhat confusing conflict in f2fs_rename().
And honestly, I really don't know what the right resolution is. What I
ended up with was this:
if (old_is_dir) {
if (old_dir_entry)
f2fs_set_link(old_inode, old_dir_entry,
old_dir_page, new_dir);
else
f2fs_put_page(old_dir_page, 0);
f2fs_i_links_write(old_dir, false);
}
which seems to me to be the right thing as a resolution. But I note
that linux-next has something different, and it is because Al said in
https://lore.kernel.org/all/20231220013402.GW1674809@ZenIV/
that the resolution should just be
if (old_dir_entry)
f2fs_set_link(old_inode, old_dir_entry, old_dir_page, new_dir);
if (old_is_dir)
f2fs_i_links_write(old_dir, false);
instead.
Now, some of those differences are artificial - old_dir_entry can only
be set if old_is_dir is set, so the nesting difference is kind of a
red herring.
But I feel like that f2fs_put_page() is actually needed, or you end up
with a reference leak.
So despite the fact that Al is never wrong, I ended up going with my
gut, and kept my resolution that is different from linux-next.
End result: I'm now very leery of my merge. On the one hand, I think
it's right. On the other hand, the likelihood that Al is wrong is
pretty low.
So please double- and triple-check that merge, and please send in a
fix for it. Presumably with a comment along the lines of "Al was
right, don't try to overthink things".
Hubris. That's the word for thinking you know better than Al.
Linus
The pull request you sent on Thu, 11 Jan 2024 10:28:10 -0800:
> git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/70d201a40823acba23899342d62bc2644051ad2e
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html
On Thu, Jan 11, 2024 at 09:05:51PM -0800, Linus Torvalds wrote:
> On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <[email protected]> wrote:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
>
> Hmm. I got a somewhat confusing conflict in f2fs_rename().
>
> And honestly, I really don't know what the right resolution is. What I
> ended up with was this:
>
> if (old_is_dir) {
> if (old_dir_entry)
> f2fs_set_link(old_inode, old_dir_entry,
> old_dir_page, new_dir);
> else
> f2fs_put_page(old_dir_page, 0);
Where would you end up with old_dir_page != NULL and old_dir_entry == NULL?
old_dir_page is initialized to NULL and the only place where it's altered
is
old_dir_entry = f2fs_parent_dir(old_inode, &old_dir_page);
Which is immediately followed by
if (!old_dir_entry) {
if (IS_ERR(old_dir_page))
err = PTR_ERR(old_dir_page);
goto out_old;
}
so we are *not* going to end up at that if (old_is_dir) in that case.
Original would have been more clear as
if (old_is_dir) {
if (old_dir != new_dir) {
/* we have .. in old_dir_page/old_dir_entry */
if (!whiteout)
f2fs_set_link(old_inode, old_dir_entry,
old_dir_page, new_dir);
else
f2fs_put_page(old_dir_page, 0);
}
f2fs_i_links_write(old_dir, false);
}
- it is equivalent to what that code used to do. And "don't update ..
if we are leaving a whiteout behind" was teh bug fixed by commit
in f2fs tree...
The bottom line: your variant is not broken, but only because
f2fs_put_page() starts with
static inline void f2fs_put_page(struct page *page, int unlock)
{
if (!page)
return;
IOW, you are doing f2fs_put_page(NULL, 0), which is an explicit no-op.
On 01/12, Al Viro wrote:
> On Thu, Jan 11, 2024 at 09:05:51PM -0800, Linus Torvalds wrote:
> > On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <[email protected]> wrote:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
> >
> > Hmm. I got a somewhat confusing conflict in f2fs_rename().
> >
> > And honestly, I really don't know what the right resolution is. What I
> > ended up with was this:
> >
> > if (old_is_dir) {
> > if (old_dir_entry)
> > f2fs_set_link(old_inode, old_dir_entry,
> > old_dir_page, new_dir);
> > else
> > f2fs_put_page(old_dir_page, 0);
>
> Where would you end up with old_dir_page != NULL and old_dir_entry == NULL?
> old_dir_page is initialized to NULL and the only place where it's altered
> is
> old_dir_entry = f2fs_parent_dir(old_inode, &old_dir_page);
> Which is immediately followed by
> if (!old_dir_entry) {
> if (IS_ERR(old_dir_page))
> err = PTR_ERR(old_dir_page);
> goto out_old;
> }
> so we are *not* going to end up at that if (old_is_dir) in that case.
It seems [1] changed the condition of getting old_dir_page reference as below,
which made f2fs_put_page(old_dir_page, 0) voided.
- if (S_ISDIR(old_inode->i_mode)) {
+ if (old_is_dir && old_dir != new_dir) {
old_dir_entry = f2fs_parent_dir(old_inode, &old_dir_page);
if (!old_dir_entry) {
if (IS_ERR(old_dir_page))
[1] 7deee77b993a ("f2fs: Avoid reading renamed directory if parent does not change")
>
> Original would have been more clear as
> if (old_is_dir) {
> if (old_dir != new_dir) {
> /* we have .. in old_dir_page/old_dir_entry */
> if (!whiteout)
> f2fs_set_link(old_inode, old_dir_entry,
> old_dir_page, new_dir);
> else
> f2fs_put_page(old_dir_page, 0);
> }
> f2fs_i_links_write(old_dir, false);
> }
> - it is equivalent to what that code used to do. And "don't update ..
> if we are leaving a whiteout behind" was teh bug fixed by commit
> in f2fs tree...
>
> The bottom line: your variant is not broken, but only because
> f2fs_put_page() starts with
> static inline void f2fs_put_page(struct page *page, int unlock)
> {
> if (!page)
> return;
>
> IOW, you are doing f2fs_put_page(NULL, 0), which is an explicit no-op.
Posted this.
https://lore.kernel.org/lkml/[email protected]/T/#u
On 01/12, Jaegeuk Kim wrote:
> On 01/12, Al Viro wrote:
> > On Thu, Jan 11, 2024 at 09:05:51PM -0800, Linus Torvalds wrote:
> > > On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <[email protected]> wrote:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/jaegeuk/f2fs.git tags/f2fs-for-6.8-rc1
> > >
> > > Hmm. I got a somewhat confusing conflict in f2fs_rename().
> > >
> > > And honestly, I really don't know what the right resolution is. What I
> > > ended up with was this:
> > >
> > > if (old_is_dir) {
> > > if (old_dir_entry)
> > > f2fs_set_link(old_inode, old_dir_entry,
> > > old_dir_page, new_dir);
> > > else
> > > f2fs_put_page(old_dir_page, 0);
> >
> > Where would you end up with old_dir_page != NULL and old_dir_entry == NULL?
> > old_dir_page is initialized to NULL and the only place where it's altered
> > is
> > old_dir_entry = f2fs_parent_dir(old_inode, &old_dir_page);
> > Which is immediately followed by
> > if (!old_dir_entry) {
> > if (IS_ERR(old_dir_page))
> > err = PTR_ERR(old_dir_page);
> > goto out_old;
> > }
> > so we are *not* going to end up at that if (old_is_dir) in that case.
>
> It seems [1] changed the condition of getting old_dir_page reference as below,
> which made f2fs_put_page(old_dir_page, 0) voided.
>
> - if (S_ISDIR(old_inode->i_mode)) {
> + if (old_is_dir && old_dir != new_dir) {
> old_dir_entry = f2fs_parent_dir(old_inode, &old_dir_page);
> if (!old_dir_entry) {
> if (IS_ERR(old_dir_page))
>
> [1] 7deee77b993a ("f2fs: Avoid reading renamed directory if parent does not change")
>
> >
> > Original would have been more clear as
> > if (old_is_dir) {
> > if (old_dir != new_dir) {
> > /* we have .. in old_dir_page/old_dir_entry */
> > if (!whiteout)
> > f2fs_set_link(old_inode, old_dir_entry,
> > old_dir_page, new_dir);
> > else
> > f2fs_put_page(old_dir_page, 0);
> > }
> > f2fs_i_links_write(old_dir, false);
> > }
> > - it is equivalent to what that code used to do. And "don't update ..
> > if we are leaving a whiteout behind" was teh bug fixed by commit
> > in f2fs tree...
> >
> > The bottom line: your variant is not broken, but only because
> > f2fs_put_page() starts with
> > static inline void f2fs_put_page(struct page *page, int unlock)
> > {
> > if (!page)
> > return;
> >
> > IOW, you are doing f2fs_put_page(NULL, 0), which is an explicit no-op.
On Thu, 11 Jan 2024 at 23:12, Al Viro <[email protected]> wrote:
>
> Where would you end up with old_dir_page != NULL and old_dir_entry == NULL?
D'oh.
You are of course right, and I missed that connection. Happily my
merge still works, just isn't as minimal as yours.
I see that Jaegeuk already posted the patch for the cleanup.
Linus
Hello:
This pull request was applied to jaegeuk/f2fs.git (dev)
by Linus Torvalds <[email protected]>:
On Thu, 11 Jan 2024 10:28:10 -0800 you wrote:
> Hi Linus,
>
> Happy new year!
>
> Could you please consider this pull request?
>
> Thank you.
>
> [...]
Here is the summary with links:
- [f2fs-dev,GIT,PULL] f2fs update for 6.8-rc1
https://git.kernel.org/jaegeuk/f2fs/c/70d201a40823
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html