Messages in this thread | | | Date | Fri, 12 Jan 2024 09:19:35 -0800 | From | Jaegeuk Kim <> | Subject | Re: [GIT PULL] f2fs update for 6.8-rc1 |
| |
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.
| |