* RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
2020-09-28 7:36 ` Sungjong Seo
@ 2020-09-28 7:49 ` Namjae Jeon
2020-09-30 10:41 ` Tetsuhiro Kohada
2020-09-30 4:01 ` Tetsuhiro Kohada
2020-09-30 9:08 ` Tetsuhiro Kohada
2 siblings, 1 reply; 8+ messages in thread
From: Namjae Jeon @ 2020-09-28 7:49 UTC (permalink / raw)
To: 'Tetsuhiro Kohada'
Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, linux-fsdevel,
linux-kernel, 'Sungjong Seo'
> > >> --- 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
2020-09-28 7:49 ` Namjae Jeon
@ 2020-09-30 10:41 ` Tetsuhiro Kohada
0 siblings, 0 replies; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-09-30 10:41 UTC (permalink / raw)
To: Namjae Jeon
Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, linux-fsdevel,
linux-kernel, 'Sungjong Seo'
>>> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
2020-09-28 7:36 ` Sungjong Seo
2020-09-28 7:49 ` Namjae Jeon
@ 2020-09-30 4:01 ` Tetsuhiro Kohada
2020-09-30 9:08 ` Tetsuhiro Kohada
2 siblings, 0 replies; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-09-30 4:01 UTC (permalink / raw)
To: Sungjong Seo
Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
'Namjae Jeon', linux-fsdevel, linux-kernel
>>> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
2020-09-28 7:36 ` Sungjong Seo
2020-09-28 7:49 ` Namjae Jeon
2020-09-30 4:01 ` Tetsuhiro Kohada
@ 2020-09-30 9:08 ` Tetsuhiro Kohada
2 siblings, 0 replies; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-09-30 9:08 UTC (permalink / raw)
To: Sungjong Seo
Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
'Namjae Jeon', linux-fsdevel, linux-kernel
>> 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>
^ permalink raw reply [flat|nested] 8+ messages in thread