1. Patchew
  2. linux
  3. View series

[PATCH] f2fs: don't traverse directory blocks after EOF

Chao Yu posted 1 patch 1 year, 5 months ago 
fs/f2fs/dir.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Chao Yu 1 year, 5 months ago 
All directory blocks are within the scope of i_size, so let's limit
the end_block to just check valid dirent blocks.

Meanwhile, it uses dir_blocks() instead of variable for cleanup in
__f2fs_find_entry().

Signed-off-by: Chao Yu <chao@kernel.org>
---
 fs/f2fs/dir.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index 02c9355176d3..d4591c215f07 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
 	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
 	unsigned int nbucket, nblock;
 	unsigned int bidx, end_block;
+	unsigned long last_block;
 	struct page *dentry_page;
 	struct f2fs_dir_entry *de = NULL;
 	pgoff_t next_pgofs;
 	bool room = false;
 	int max_slots;
 
+	last_block = dir_blocks(dir);
 	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
 	nblock = bucket_blocks(level);
 
 	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
 			       le32_to_cpu(fname->hash) % nbucket);
 	end_block = bidx + nblock;
+	end_block = min_t(unsigned int, end_block, last_block);
 
 	while (bidx < end_block) {
 		/* no need to allocate new dentry pages to all the indices */
@@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
 					 const struct f2fs_filename *fname,
 					 struct page **res_page)
 {
-	unsigned long npages = dir_blocks(dir);
 	struct f2fs_dir_entry *de = NULL;
 	unsigned int max_depth;
 	unsigned int level;
@@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
 		goto out;
 	}
 
-	if (npages == 0)
+	if (dir_blocks(dir) == 0)
 		goto out;
 
 	max_depth = F2FS_I(dir)->i_current_depth;
-- 
2.40.1
Re: [PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Jaegeuk Kim 1 year, 4 months ago 
On 07/12, Chao Yu wrote:
> All directory blocks are within the scope of i_size, so let's limit
> the end_block to just check valid dirent blocks.

Do we really need this?

> 
> Meanwhile, it uses dir_blocks() instead of variable for cleanup in
> __f2fs_find_entry().
> 
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
>  fs/f2fs/dir.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 02c9355176d3..d4591c215f07 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
>  	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
>  	unsigned int nbucket, nblock;
>  	unsigned int bidx, end_block;
> +	unsigned long last_block;
>  	struct page *dentry_page;
>  	struct f2fs_dir_entry *de = NULL;
>  	pgoff_t next_pgofs;
>  	bool room = false;
>  	int max_slots;
>  
> +	last_block = dir_blocks(dir);
>  	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
>  	nblock = bucket_blocks(level);
>  
>  	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
>  			       le32_to_cpu(fname->hash) % nbucket);
>  	end_block = bidx + nblock;
> +	end_block = min_t(unsigned int, end_block, last_block);
>  
>  	while (bidx < end_block) {
>  		/* no need to allocate new dentry pages to all the indices */
> @@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>  					 const struct f2fs_filename *fname,
>  					 struct page **res_page)
>  {
> -	unsigned long npages = dir_blocks(dir);
>  	struct f2fs_dir_entry *de = NULL;
>  	unsigned int max_depth;
>  	unsigned int level;
> @@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>  		goto out;
>  	}
>  
> -	if (npages == 0)
> +	if (dir_blocks(dir) == 0)
>  		goto out;
>  
>  	max_depth = F2FS_I(dir)->i_current_depth;
> -- 
> 2.40.1
Re: [PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Chao Yu 1 year, 4 months ago 
On 2024/7/26 0:55, Jaegeuk Kim wrote:
> On 07/12, Chao Yu wrote:
>> All directory blocks are within the scope of i_size, so let's limit
>> the end_block to just check valid dirent blocks.
> 
> Do we really need this?

f2fs_readdir() and f2fs_empty_dir() uses dir_blocks() for upper boundary,
this patch aligns find_in_level() w/ them.

Also, it can avoid grabbing never used page cache across EOF.

So, we can consider taking this patch?

Thanks,

> 
>>
>> Meanwhile, it uses dir_blocks() instead of variable for cleanup in
>> __f2fs_find_entry().
>>
>> Signed-off-by: Chao Yu <chao@kernel.org>
>> ---
>>   fs/f2fs/dir.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>> index 02c9355176d3..d4591c215f07 100644
>> --- a/fs/f2fs/dir.c
>> +++ b/fs/f2fs/dir.c
>> @@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
>>   	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
>>   	unsigned int nbucket, nblock;
>>   	unsigned int bidx, end_block;
>> +	unsigned long last_block;
>>   	struct page *dentry_page;
>>   	struct f2fs_dir_entry *de = NULL;
>>   	pgoff_t next_pgofs;
>>   	bool room = false;
>>   	int max_slots;
>>   
>> +	last_block = dir_blocks(dir);
>>   	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
>>   	nblock = bucket_blocks(level);
>>   
>>   	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
>>   			       le32_to_cpu(fname->hash) % nbucket);
>>   	end_block = bidx + nblock;
>> +	end_block = min_t(unsigned int, end_block, last_block);
>>   
>>   	while (bidx < end_block) {
>>   		/* no need to allocate new dentry pages to all the indices */
>> @@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>>   					 const struct f2fs_filename *fname,
>>   					 struct page **res_page)
>>   {
>> -	unsigned long npages = dir_blocks(dir);
>>   	struct f2fs_dir_entry *de = NULL;
>>   	unsigned int max_depth;
>>   	unsigned int level;
>> @@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>>   		goto out;
>>   	}
>>   
>> -	if (npages == 0)
>> +	if (dir_blocks(dir) == 0)
>>   		goto out;
>>   
>>   	max_depth = F2FS_I(dir)->i_current_depth;
>> -- 
>> 2.40.1
Re: [PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Jaegeuk Kim 1 year, 4 months ago 
On 07/26, Chao Yu wrote:
> On 2024/7/26 0:55, Jaegeuk Kim wrote:
> > On 07/12, Chao Yu wrote:
> > > All directory blocks are within the scope of i_size, so let's limit
> > > the end_block to just check valid dirent blocks.
> > 
> > Do we really need this?
> 
> f2fs_readdir() and f2fs_empty_dir() uses dir_blocks() for upper boundary,
> this patch aligns find_in_level() w/ them.
> 
> Also, it can avoid grabbing never used page cache across EOF.
> 
> So, we can consider taking this patch?

I'm wondering whether the current code has a bug or not.

> 
> Thanks,
> 
> > 
> > > 
> > > Meanwhile, it uses dir_blocks() instead of variable for cleanup in
> > > __f2fs_find_entry().
> > > 
> > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > ---
> > >   fs/f2fs/dir.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > index 02c9355176d3..d4591c215f07 100644
> > > --- a/fs/f2fs/dir.c
> > > +++ b/fs/f2fs/dir.c
> > > @@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
> > >   	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
> > >   	unsigned int nbucket, nblock;
> > >   	unsigned int bidx, end_block;
> > > +	unsigned long last_block;
> > >   	struct page *dentry_page;
> > >   	struct f2fs_dir_entry *de = NULL;
> > >   	pgoff_t next_pgofs;
> > >   	bool room = false;
> > >   	int max_slots;
> > > +	last_block = dir_blocks(dir);
> > >   	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
> > >   	nblock = bucket_blocks(level);
> > >   	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
> > >   			       le32_to_cpu(fname->hash) % nbucket);
> > >   	end_block = bidx + nblock;
> > > +	end_block = min_t(unsigned int, end_block, last_block);
> > >   	while (bidx < end_block) {
> > >   		/* no need to allocate new dentry pages to all the indices */
> > > @@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
> > >   					 const struct f2fs_filename *fname,
> > >   					 struct page **res_page)
> > >   {
> > > -	unsigned long npages = dir_blocks(dir);
> > >   	struct f2fs_dir_entry *de = NULL;
> > >   	unsigned int max_depth;
> > >   	unsigned int level;
> > > @@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
> > >   		goto out;
> > >   	}
> > > -	if (npages == 0)
> > > +	if (dir_blocks(dir) == 0)
> > >   		goto out;
> > >   	max_depth = F2FS_I(dir)->i_current_depth;
> > > -- 
> > > 2.40.1
Re: [PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Chao Yu 1 year, 4 months ago 
On 2024/7/27 11:27, Jaegeuk Kim wrote:
> On 07/26, Chao Yu wrote:
>> On 2024/7/26 0:55, Jaegeuk Kim wrote:
>>> On 07/12, Chao Yu wrote:
>>>> All directory blocks are within the scope of i_size, so let's limit
>>>> the end_block to just check valid dirent blocks.
>>>
>>> Do we really need this?
>>
>> f2fs_readdir() and f2fs_empty_dir() uses dir_blocks() for upper boundary,
>> this patch aligns find_in_level() w/ them.
>>
>> Also, it can avoid grabbing never used page cache across EOF.
>>
>> So, we can consider taking this patch?
> 
> I'm wondering whether the current code has a bug or not.

I think it's not a bug, but, in corner case it may waste memory and
cause a bit performance reduction.

Thanks,

> 
>>
>> Thanks,
>>
>>>
>>>>
>>>> Meanwhile, it uses dir_blocks() instead of variable for cleanup in
>>>> __f2fs_find_entry().
>>>>
>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>> ---
>>>>    fs/f2fs/dir.c | 6 ++++--
>>>>    1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
>>>> index 02c9355176d3..d4591c215f07 100644
>>>> --- a/fs/f2fs/dir.c
>>>> +++ b/fs/f2fs/dir.c
>>>> @@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
>>>>    	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
>>>>    	unsigned int nbucket, nblock;
>>>>    	unsigned int bidx, end_block;
>>>> +	unsigned long last_block;
>>>>    	struct page *dentry_page;
>>>>    	struct f2fs_dir_entry *de = NULL;
>>>>    	pgoff_t next_pgofs;
>>>>    	bool room = false;
>>>>    	int max_slots;
>>>> +	last_block = dir_blocks(dir);
>>>>    	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
>>>>    	nblock = bucket_blocks(level);
>>>>    	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
>>>>    			       le32_to_cpu(fname->hash) % nbucket);
>>>>    	end_block = bidx + nblock;
>>>> +	end_block = min_t(unsigned int, end_block, last_block);
>>>>    	while (bidx < end_block) {
>>>>    		/* no need to allocate new dentry pages to all the indices */
>>>> @@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>>>>    					 const struct f2fs_filename *fname,
>>>>    					 struct page **res_page)
>>>>    {
>>>> -	unsigned long npages = dir_blocks(dir);
>>>>    	struct f2fs_dir_entry *de = NULL;
>>>>    	unsigned int max_depth;
>>>>    	unsigned int level;
>>>> @@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
>>>>    		goto out;
>>>>    	}
>>>> -	if (npages == 0)
>>>> +	if (dir_blocks(dir) == 0)
>>>>    		goto out;
>>>>    	max_depth = F2FS_I(dir)->i_current_depth;
>>>> -- 
>>>> 2.40.1
Re: [PATCH] f2fs: don't traverse directory blocks after EOF
Posted by Jaegeuk Kim 1 year, 4 months ago 
On 07/29, Chao Yu wrote:
> On 2024/7/27 11:27, Jaegeuk Kim wrote:
> > On 07/26, Chao Yu wrote:
> > > On 2024/7/26 0:55, Jaegeuk Kim wrote:
> > > > On 07/12, Chao Yu wrote:
> > > > > All directory blocks are within the scope of i_size, so let's limit
> > > > > the end_block to just check valid dirent blocks.
> > > > 
> > > > Do we really need this?
> > > 
> > > f2fs_readdir() and f2fs_empty_dir() uses dir_blocks() for upper boundary,
> > > this patch aligns find_in_level() w/ them.
> > > 
> > > Also, it can avoid grabbing never used page cache across EOF.
> > > 
> > > So, we can consider taking this patch?
> > 
> > I'm wondering whether the current code has a bug or not.
> 
> I think it's not a bug, but, in corner case it may waste memory and
> cause a bit performance reduction.

Well, I don't think it's really a problem.

> 
> Thanks,
> 
> > 
> > > 
> > > Thanks,
> > > 
> > > > 
> > > > > 
> > > > > Meanwhile, it uses dir_blocks() instead of variable for cleanup in
> > > > > __f2fs_find_entry().
> > > > > 
> > > > > Signed-off-by: Chao Yu <chao@kernel.org>
> > > > > ---
> > > > >    fs/f2fs/dir.c | 6 ++++--
> > > > >    1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> > > > > index 02c9355176d3..d4591c215f07 100644
> > > > > --- a/fs/f2fs/dir.c
> > > > > +++ b/fs/f2fs/dir.c
> > > > > @@ -305,18 +305,21 @@ static struct f2fs_dir_entry *find_in_level(struct inode *dir,
> > > > >    	int s = GET_DENTRY_SLOTS(fname->disk_name.len);
> > > > >    	unsigned int nbucket, nblock;
> > > > >    	unsigned int bidx, end_block;
> > > > > +	unsigned long last_block;
> > > > >    	struct page *dentry_page;
> > > > >    	struct f2fs_dir_entry *de = NULL;
> > > > >    	pgoff_t next_pgofs;
> > > > >    	bool room = false;
> > > > >    	int max_slots;
> > > > > +	last_block = dir_blocks(dir);
> > > > >    	nbucket = dir_buckets(level, F2FS_I(dir)->i_dir_level);
> > > > >    	nblock = bucket_blocks(level);
> > > > >    	bidx = dir_block_index(level, F2FS_I(dir)->i_dir_level,
> > > > >    			       le32_to_cpu(fname->hash) % nbucket);
> > > > >    	end_block = bidx + nblock;
> > > > > +	end_block = min_t(unsigned int, end_block, last_block);
> > > > >    	while (bidx < end_block) {
> > > > >    		/* no need to allocate new dentry pages to all the indices */
> > > > > @@ -361,7 +364,6 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
> > > > >    					 const struct f2fs_filename *fname,
> > > > >    					 struct page **res_page)
> > > > >    {
> > > > > -	unsigned long npages = dir_blocks(dir);
> > > > >    	struct f2fs_dir_entry *de = NULL;
> > > > >    	unsigned int max_depth;
> > > > >    	unsigned int level;
> > > > > @@ -373,7 +375,7 @@ struct f2fs_dir_entry *__f2fs_find_entry(struct inode *dir,
> > > > >    		goto out;
> > > > >    	}
> > > > > -	if (npages == 0)
> > > > > +	if (dir_blocks(dir) == 0)
> > > > >    		goto out;
> > > > >    	max_depth = F2FS_I(dir)->i_current_depth;
> > > > > -- 
> > > > > 2.40.1

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.