| Message ID | 20200911044506.13912-1-kohada.t2@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] exfat: remove useless directory scan in exfat_add_entry() | expand |
> In exfat_move_file(), the identity of source and target directory has been > checked by the caller. > Also, it gets stream.start_clu from file dir-entry, which is an invalid > determination. > > Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> > --- > fs/exfat/namei.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index > 803748946ddb..1c433491f771 100644 > --- a/fs/exfat/namei.c > +++ b/fs/exfat/namei.c > @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode *inode, > struct exfat_chain *p_olddir, > if (!epmov) > return -EIO; > > - /* check if the source and target directory is the same */ > - if (exfat_get_entry_type(epmov) == TYPE_DIR && > - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) > - return -EINVAL; > - It might check if the cluster numbers are same between source entry and target directory. Could you let me know what code you mentioned? Or do you mean the codes on vfs? > num_old_entries = exfat_count_ext_entries(sb, p_olddir, oldentry, > epmov); > if (num_old_entries < 0) > -- > 2.25.1
>> --- a/fs/exfat/namei.c >> +++ b/fs/exfat/namei.c >> @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode *inode, >> struct exfat_chain *p_olddir, >> if (!epmov) >> return -EIO; >> >> - /* check if the source and target directory is the same */ >> - if (exfat_get_entry_type(epmov) == TYPE_DIR && >> - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) >> - return -EINVAL; >> - > > It might check if the cluster numbers are same between source entry and > target directory. This checks if newdir is the move target itself. Example: mv /mnt/dir0 /mnt/dir0/foo However, this check is not enough. We need to check newdir and all ancestors. Example: mv /mnt/dir0 /mnt/dir0/dir1/foo mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo ... This is probably a taboo for all layered filesystems. > Could you let me know what code you mentioned? > Or do you mean the codes on vfs? You can find in do_renameat2(). --- around 'fs/namei.c:4440' If the destination ancestors are itself, our driver will not be called. BTW Are you busy now? I am waiting for your reply about "integrates dir-entry getting and validation" patch. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
> >> --- a/fs/exfat/namei.c > >> +++ b/fs/exfat/namei.c > >> @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode > >> *inode, struct exfat_chain *p_olddir, > >> if (!epmov) > >> return -EIO; > >> > >> - /* check if the source and target directory is the same */ > >> - if (exfat_get_entry_type(epmov) == TYPE_DIR && > >> - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) > >> - return -EINVAL; > >> - > > > > It might check if the cluster numbers are same between source entry > > and target directory. > > This checks if newdir is the move target itself. > Example: > mv /mnt/dir0 /mnt/dir0/foo > > However, this check is not enough. > We need to check newdir and all ancestors. > Example: > mv /mnt/dir0 /mnt/dir0/dir1/foo > mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo > ... > > This is probably a taboo for all layered filesystems. > > > > Could you let me know what code you mentioned? > > Or do you mean the codes on vfs? > > You can find in do_renameat2(). --- around 'fs/namei.c:4440' > If the destination ancestors are itself, our driver will not be called. I think, of course, vfs has been doing that. So that code is unnecessary in normal situations. That code comes from the old exfat implementation. And as far as I understand, it seems to check once more "the cluster number" even though it comes through vfs so that it tries detecting abnormal of on-disk. Anyway, I agonized if it is really needed. In conclusion, old code could be eliminated and your patch looks reasonable. Thanks Acked-by: Sungjong Seo <sj1557.seo@samsung.com> > > > BTW > Are you busy now? I'm sorry, I'm so busy for my full time work :( Anyway, I'm trying to review serious bug patches or bug reports first. Other patches, such as clean-up or code refactoring, may take some time to review. > I am waiting for your reply about "integrates dir-entry getting and > validation" patch. As I know, your patch is being under review by Namjae. > > BR > --- > Tetsuhiro Kohada <kohada.t2@gmail.com>
> > >> --- a/fs/exfat/namei.c > > >> +++ b/fs/exfat/namei.c > > >> @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode > > >> *inode, struct exfat_chain *p_olddir, > > >> if (!epmov) > > >> return -EIO; > > >> > > >> - /* check if the source and target directory is the same */ > > >> - if (exfat_get_entry_type(epmov) == TYPE_DIR && > > >> - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) > > >> - return -EINVAL; > > >> - > > > > > > It might check if the cluster numbers are same between source entry > > > and target directory. > > > > This checks if newdir is the move target itself. > > Example: > > mv /mnt/dir0 /mnt/dir0/foo > > > > However, this check is not enough. > > We need to check newdir and all ancestors. > > Example: > > mv /mnt/dir0 /mnt/dir0/dir1/foo > > mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo > > ... > > > > This is probably a taboo for all layered filesystems. > > > > > > > Could you let me know what code you mentioned? > > > Or do you mean the codes on vfs? > > > > You can find in do_renameat2(). --- around 'fs/namei.c:4440' > > If the destination ancestors are itself, our driver will not be called. > > I think, of course, vfs has been doing that. > So that code is unnecessary in normal situations. > > That code comes from the old exfat implementation. > And as far as I understand, it seems to check once more "the cluster number" > even though it comes through vfs so that it tries detecting abnormal of on-disk. > > Anyway, I agonized if it is really needed. > In conclusion, old code could be eliminated and your patch looks reasonable. > Thanks > > Acked-by: Sungjong Seo <sj1557.seo@samsung.com> > > > > > > > BTW > > Are you busy now? > I'm sorry, I'm so busy for my full time work :( Anyway, I'm trying to review serious bug patches or > bug reports first. > Other patches, such as clean-up or code refactoring, may take some time to review. > > > I am waiting for your reply about "integrates dir-entry getting and > > validation" patch. > As I know, your patch is being under review by Namjae. I already gave comments and a patch, but you said you can't do it. I'm sorry, But I can't accept an incomplete patch. I will directly fix it later. > > > > > BR > > --- > > Tetsuhiro Kohada <kohada.t2@gmail.com>
>>> It might check if the cluster numbers are same between source entry >>> and target directory. >> >> This checks if newdir is the move target itself. >> Example: >> mv /mnt/dir0 /mnt/dir0/foo >> >> However, this check is not enough. >> We need to check newdir and all ancestors. >> Example: >> mv /mnt/dir0 /mnt/dir0/dir1/foo >> mv /mnt/dir0 /mnt/dir0/dir1/dir2/foo >> ... >> >> This is probably a taboo for all layered filesystems. >> >> >>> Could you let me know what code you mentioned? >>> Or do you mean the codes on vfs? >> >> You can find in do_renameat2(). --- around 'fs/namei.c:4440' >> If the destination ancestors are itself, our driver will not be called. > > I think, of course, vfs has been doing that. > So that code is unnecessary in normal situations. > > That code comes from the old exfat implementation. It could be a remnant of another system. Once upon a time, I moved the dir to a descendant dir without implementing this check and it disappeared forever. linux-VFS fixed this issue immediately, but some systems still need to be checked by the driver itself. (ex.Windows-IFS) > And as far as I understand, it seems to check once more "the cluster number" > even though it comes through vfs so that it tries detecting abnormal of on-disk. > > Anyway, I agonized if it is really needed. > In conclusion, old code could be eliminated and your patch looks reasonable. It's easy to add, but it's really hard to remove the ancient code. BTW I have a question for you. Now, I'm trying to optimize exfat_get_dentry(). However, exfat_get_dentry() is used a lot, so the patch is also large. In such a case -Replace old implementation with new ones with a single patch. -Devide multiple patches in which old functions and new functions (ex. exfat_get_dentry2) coexist temporarily. And finally clean up. I understand that a small patch is desirable, but the latter has "two similar functions". Which is better for you to review the patch? BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
>> Are you busy now? > I'm sorry, I'm so busy for my full time work :( > Anyway, I'm trying to review serious bug patches or bug reports first. > Other patches, such as clean-up or code refactoring, may take some time to review. I'll try to reduce your burden as much as possible. >> I am waiting for your reply about "integrates dir-entry getting and >> validation" patch. > As I know, your patch is being under review by Namjae. OK. I'll discuss it with him. If possible, please let me know your opinion. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
>>> BTW >>> Are you busy now? >> I'm sorry, I'm so busy for my full time work :( Anyway, I'm trying to review serious bug patches or >> bug reports first. >> Other patches, such as clean-up or code refactoring, may take some time to review. >> >>> I am waiting for your reply about "integrates dir-entry getting and >>> validation" patch. >> As I know, your patch is being under review by Namjae. > I already gave comments and a patch, but you said you can't do it. > I'm sorry, But I can't accept an incomplete patch. I will directly fix it later. Of course, I understand that you can't accept a under-discussed patch. I think you know what I'm trying to do, with previous patches. Unfortunately, I couldn't implement it properly using the patch you provided. But I don't think the checksum and name-lenth issues should be left unresolved. (How do you think?) So I want you to think with me. I still feel we haven't discussed this enough. I still don't understand what you think is the problem with the patch. Where and what kind of problems do you think the patch has? - performance? - wrong behavior? - readability? - runtime cost? - style? - other? I think I explained the reason for each implementation. If it's not enough, I'd like to explain it in more detail. BR --- Tetsuhiro Kohada <kohada.t2@gmail.com>
diff --git a/fs/exfat/namei.c b/fs/exfat/namei.c index 803748946ddb..1c433491f771 100644 --- a/fs/exfat/namei.c +++ b/fs/exfat/namei.c @@ -1095,11 +1095,6 @@ static int exfat_move_file(struct inode *inode, struct exfat_chain *p_olddir, if (!epmov) return -EIO; - /* check if the source and target directory is the same */ - if (exfat_get_entry_type(epmov) == TYPE_DIR && - le32_to_cpu(epmov->dentry.stream.start_clu) == p_newdir->dir) - return -EINVAL; - num_old_entries = exfat_count_ext_entries(sb, p_olddir, oldentry, epmov); if (num_old_entries < 0)
In exfat_move_file(), the identity of source and target directory has been checked by the caller. Also, it gets stream.start_clu from file dir-entry, which is an invalid determination. Signed-off-by: Tetsuhiro Kohada <kohada.t2@gmail.com> --- fs/exfat/namei.c | 5 ----- 1 file changed, 5 deletions(-)