Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 2/3] exfat: remove useless check in exfat_move_file()
@ 2020-09-11  4:45 ` Tetsuhiro Kohada
  2020-09-16  2:32   ` Sungjong Seo
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-09-11  4:45 UTC (permalink / raw)
  To: kohada.t2
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka, Namjae Jeon,
	Sungjong Seo, linux-fsdevel, linux-kernel

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;
-
 	num_old_entries = exfat_count_ext_entries(sb, p_olddir, oldentry,
 		epmov);
 	if (num_old_entries < 0)
-- 
2.25.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
  2020-09-11  4:45 ` [PATCH 2/3] exfat: remove useless check in exfat_move_file() Tetsuhiro Kohada
@ 2020-09-16  2:32   ` Sungjong Seo
  2020-09-16  9:30     ` Tetsuhiro Kohada
  0 siblings, 1 reply; 8+ messages in thread
From: Sungjong Seo @ 2020-09-16  2:32 UTC (permalink / raw)
  To: 'Tetsuhiro Kohada'
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Namjae Jeon',
	linux-fsdevel, linux-kernel

> 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



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
  2020-09-16  2:32   ` Sungjong Seo
@ 2020-09-16  9:30     ` Tetsuhiro Kohada
  2020-09-28  7:36       ` Sungjong Seo
  0 siblings, 1 reply; 8+ messages in thread
From: Tetsuhiro Kohada @ 2020-09-16  9:30 UTC (permalink / raw)
  To: Sungjong Seo
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Namjae Jeon',
	linux-fsdevel, linux-kernel

>> --- 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>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* RE: [PATCH 2/3] exfat: remove useless check in exfat_move_file()
  2020-09-16  9:30     ` Tetsuhiro Kohada
@ 2020-09-28  7:36       ` Sungjong Seo
  2020-09-28  7:49         ` Namjae Jeon
                           ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Sungjong Seo @ 2020-09-28  7:36 UTC (permalink / raw)
  To: 'Tetsuhiro Kohada'
  Cc: kohada.tetsuhiro, mori.takahiro, motai.hirotaka,
	'Namjae Jeon',
	linux-fsdevel, linux-kernel

> >> --- 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>


^ 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 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: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

* 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

end of thread, other threads:[~2020-09-30 10:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20200911044511epcas1p4d62863352e65c534cd6080dd38d54b26@epcas1p4.samsung.com>
2020-09-11  4:45 ` [PATCH 2/3] exfat: remove useless check in exfat_move_file() Tetsuhiro Kohada
2020-09-16  2:32   ` Sungjong Seo
2020-09-16  9:30     ` Tetsuhiro Kohada
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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).