| Message ID | ZaAzOgd3iWL0feTU@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 70d201a40823acba23899342d62bc2644051ad2e |
| Headers | show |
| Series | [f2fs-dev,GIT,PULL] f2fs update for 6.8-rc1 | expand |
On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <jaegeuk@kernel.org> 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!
On Thu, Jan 11, 2024 at 09:05:51PM -0800, Linus Torvalds wrote: > On Thu, 11 Jan 2024 at 10:28, Jaegeuk Kim <jaegeuk@kernel.org> 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 <jaegeuk@kernel.org> 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/20240112171645.3929428-1-jaegeuk@kernel.org/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 <jaegeuk@kernel.org> 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 <viro@zeniv.linux.org.uk> 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 <torvalds@linux-foundation.org>: 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!
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(-)