> On Jul 19, 2021, at 2:03 AM, Chung-Chiang Cheng <cccheng@xxxxxxxxxxxx> wrote: > > This function revalidates dentries without blocking and storing to the > dentry. As the document mentioned [1], I think it's safe in rcu-walk > mode. I also found jfs_ci_revalidate() takes the same approach. > > d_revalidate may be called in rcu-walk mode (flags & LOOKUP_RCU). > If in rcu-walk mode, the filesystem must revalidate the dentry without > blocking or storing to the dentry, d_parent and d_inode should not be > used without care (because they can change and, in d_inode case, even > become NULL under us > > > [1] https://www.kernel.org/doc/Documentation/filesystems/vfs.txt > I am still not convinced by the explanation. >> This patch takes the same approach to drop negative dentires as vfat does. You mentioned that you follows by vfat approach. But this code contains this code, as far as I can see. How could you prove that we will not introduce some weird bug here? What if code of this function will be changed in the future? I suppose that missing of this code could be the way to introduce some bug, anyway. >> touch aaa >> rm aaa >> touch AAA By the way, have you tested other possible combinations? I mean (1) ‘aaa’ -> ‘AAA’, (2) ‘AAA’ -> ‘aaa’, (3) ‘aaa’ -> ‘aaa’, (4) ‘AAA’ -> ‘AAA’. Could you please add in the comment that it was tested? Could we create the file in case-insensitive mode and, then, try to delete in case-sensitive and vise versa? Do we define this flag during volume creation? Can we change the flag by volume tuning? Thanks, Slava. > Thanks, > C.C.Cheng > >>> + >>> +int hfsplus_revalidate_dentry(struct dentry *dentry, unsigned int flags) >>> +{ >> What’s about this code? >> >> If (flags & LOOKUP_RCU) >> return -ECHILD; >> >> Do we really need to miss it here? >> >> Thanks, >> Slava. >> >> >>> + /* >>> + * dentries are always valid when disabling casefold. >>> + */ >>> + if (!test_bit(HFSPLUS_SB_CASEFOLD, &HFSPLUS_SB(dentry->d_sb)->flags)) >>> + return 1; >>> + >>> + /* >>> + * Positive dentries are valid when enabling casefold. >>> + * >>> + * Note, rename() to existing directory entry will have ->d_inode, and >>> + * will use existing name which isn't specified name by user. >>> + * >>> + * We may be able to drop this positive dentry here. But dropping >>> + * positive dentry isn't good idea. So it's unsupported like >>> + * rename("filename", "FILENAME") for now. >>> + */ >>> + if (d_really_is_positive(dentry)) >>> + return 1; >>> + >>> + /* >>> + * Drop the negative dentry, in order to make sure to use the case >>> + * sensitive name which is specified by user if this is for creation. >>> + */ >>> + if (flags & (LOOKUP_CREATE | LOOKUP_RENAME_TARGET)) >>> + return 0; >>> + >>> + return 1; >>> +} >>> -- >>> 2.25.1 >>>
