Re: [PATCH] f2fs: allow readpages with NULL file pointer

Re: [PATCH] f2fs: allow readpages with NULL file pointer

[Hsiang Kao via Linux-f2fs-devel]

Hi Chao,

On 2017/9/23 20:50, Chao Yu wrote:
> Hi Xiang Gao,
>
> On 2017/9/22 9:54, gaoxiang (P) wrote:
> > Hi Chao,
> >
> > Thanks for your reply.
> > This patch originates from the our sdcardfs compatible 
> case-insensitive lookup acceleration.
> >
> > For the customized case-insensitive create operation since f2fs has 
> no hash and it needs to decrypt all the dirent name,
> > it is recommended to read ahead the whole dir data in advance.
> >
> > However I found f2fs cannot use page_cache_sync_readahead with null 
> @file pointer as the other common Linux file system,
> > this feature is something like a  bonus and I know that Linux has no 
> written words that we should support null @file pointer, yet
> > page_cache_sync_readahead is an EXPORT_SYMBOL_GPL and could be used 
> by other non-standard 3rd kernel modules and
> > of course I think it is really no problem.
>
> Looks like some filesystems store private data in file->private_data 
> for some
> purpose during ->open, and will use the data in late ->readpages. So 
> passing
> @file with NULL value will simply cause kernel bug for these filesystems.
>
> As I checked, most generic local filesystems just use mapping->host 
> instead of
> file->f_mapping->host, so look this patch again, following generic 
> filesystem
> looks not bad. ;)
>

I'm glad to hear that the community could apply it. :)

> How about just changing file->f_mapping->host to mapping->host in out 
> patch?

To my knowledge of f2fs, I am not quite sure whether f_mapping in f2fs is
really equal to mapping or not. Therefore I add a extra BUG_ON and do some
stress tests to re-confirm that.

Yes, it's fine with me that we only change file->f_mapping->host to
mapping->host.

Thanks,

>
> Thanks,
>
> >
> > Anyway, if it has no use to the community, I will apply it only on 
> the internal branch.
> > And I will also find some common examples of data page readahead. :)
> >
> > Thanks,
> >
> >> -----Original Message-----
> >> From: Chao Yu [mailto:chao@kernel.org]
> >> Sent: Thursday, September 21, 2017 10:33 PM
> >> To: gaoxiang (P) <gaoxiang25@huawei.com>; jaegeuk@kernel.org; 
> Yuchao (T)
> >> <yuchao0@huawei.com>
> >> Cc: linux-f2fs-devel@lists.sourceforge.net
> >> Subject: Re: [f2fs-dev] [PATCH] f2fs: allow readpages with NULL 
> file pointer
> >>
> >> On 2017/9/21 13:00, gaoxiang (P) wrote:
> >>> Keep in line with the other Linux file system implementations since
> >>> page_cache_sync_readahead supports NULL file pointer, and thus we can
> >>> readahead data by f2fs itself without file opening (something like the
> >>> btrfs behavior).
> >>
> >> Let's keep what it is until there is an example doing readahead 
> passing @file
> >> with NULL.
> >>
> >> Thanks,
> >>
> >>>
> >>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> >>> ---
> >>>  fs/f2fs/data.c | 4 +++-
> >>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 95f30f0..afa12f1
> >>> 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -1333,9 +1333,11 @@ static int f2fs_read_data_pages(struct file *file,
> >>>                      struct address_space *mapping,
> >>>                      struct list_head *pages, unsigned nr_pages)  {
> >>> -   struct inode *inode = file->f_mapping->host;
> >>> +   struct inode *inode = mapping->host;
> >>>      struct page *page = list_last_entry(pages, struct page, lru);
> >>>
> >>> +   if (likely(file != NULL))
> >>> +           BUG_ON(file->f_mapping != mapping);
> >>>      trace_f2fs_readpages(inode, page, nr_pages);
> >>>
> >>>      /* If the file has inline data, skip readpages */
> >>>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH] f2fs: allow readpages with NULL file pointer

[Chao Yu]

Hi Xiang Gao,

On 2017/9/22 9:54, gaoxiang (P) wrote:
> Hi Chao,
> 
> Thanks for your reply.
> This patch originates from the our sdcardfs compatible case-insensitive lookup acceleration.
> 
> For the customized case-insensitive create operation since f2fs has no hash and it needs to decrypt all the dirent name,
> it is recommended to read ahead the whole dir data in advance.
> 
> However I found f2fs cannot use page_cache_sync_readahead with null @file pointer as the other common Linux file system,
> this feature is something like a  bonus and I know that Linux has no written words that we should support null @file pointer, yet
> page_cache_sync_readahead is an EXPORT_SYMBOL_GPL and could be used by other non-standard 3rd kernel modules and
> of course I think it is really no problem.

Looks like some filesystems store private data in file->private_data for some
purpose during ->open, and will use the data in late ->readpages. So passing
@file with NULL value will simply cause kernel bug for these filesystems.

As I checked, most generic local filesystems just use mapping->host instead of
file->f_mapping->host, so look this patch again, following generic filesystem
looks not bad. ;)

How about just changing file->f_mapping->host to mapping->host in out patch?

Thanks,

> 
> Anyway, if it has no use to the community, I will apply it only on the internal branch.
> And I will also find some common examples of data page readahead. :)
> 
> Thanks,
> 
>> -----Original Message-----
>> From: Chao Yu [mailto:chao@kernel.org]
>> Sent: Thursday, September 21, 2017 10:33 PM
>> To: gaoxiang (P) <gaoxiang25@huawei.com>; jaegeuk@kernel.org; Yuchao (T)
>> <yuchao0@huawei.com>
>> Cc: linux-f2fs-devel@lists.sourceforge.net
>> Subject: Re: [f2fs-dev] [PATCH] f2fs: allow readpages with NULL file pointer
>>
>> On 2017/9/21 13:00, gaoxiang (P) wrote:
>>> Keep in line with the other Linux file system implementations since
>>> page_cache_sync_readahead supports NULL file pointer, and thus we can
>>> readahead data by f2fs itself without file opening (something like the
>>> btrfs behavior).
>>
>> Let's keep what it is until there is an example doing readahead passing @file
>> with NULL.
>>
>> Thanks,
>>
>>>
>>> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
>>> ---
>>>  fs/f2fs/data.c | 4 +++-
>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 95f30f0..afa12f1
>>> 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -1333,9 +1333,11 @@ static int f2fs_read_data_pages(struct file *file,
>>>  			struct address_space *mapping,
>>>  			struct list_head *pages, unsigned nr_pages)  {
>>> -	struct inode *inode = file->f_mapping->host;
>>> +	struct inode *inode = mapping->host;
>>>  	struct page *page = list_last_entry(pages, struct page, lru);
>>>
>>> +	if (likely(file != NULL))
>>> +		BUG_ON(file->f_mapping != mapping);
>>>  	trace_f2fs_readpages(inode, page, nr_pages);
>>>
>>>  	/* If the file has inline data, skip readpages */
>>>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: allow readpages with NULL file pointer

[gaoxiang (P)]

Hi Chao,

Thanks for your reply.
This patch originates from the our sdcardfs compatible case-insensitive lookup acceleration.

For the customized case-insensitive create operation since f2fs has no hash and it needs to decrypt all the dirent name,
it is recommended to read ahead the whole dir data in advance.

However I found f2fs cannot use page_cache_sync_readahead with null @file pointer as the other common Linux file system,
this feature is something like a  bonus and I know that Linux has no written words that we should support null @file pointer, yet
page_cache_sync_readahead is an EXPORT_SYMBOL_GPL and could be used by other non-standard 3rd kernel modules and
of course I think it is really no problem.

Anyway, if it has no use to the community, I will apply it only on the internal branch.
And I will also find some common examples of data page readahead. :)

Thanks,

> -----Original Message-----
> From: Chao Yu [mailto:chao@kernel.org]
> Sent: Thursday, September 21, 2017 10:33 PM
> To: gaoxiang (P) <gaoxiang25@huawei.com>; jaegeuk@kernel.org; Yuchao (T)
> <yuchao0@huawei.com>
> Cc: linux-f2fs-devel@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH] f2fs: allow readpages with NULL file pointer
> 
> On 2017/9/21 13:00, gaoxiang (P) wrote:
> > Keep in line with the other Linux file system implementations since
> > page_cache_sync_readahead supports NULL file pointer, and thus we can
> > readahead data by f2fs itself without file opening (something like the
> > btrfs behavior).
> 
> Let's keep what it is until there is an example doing readahead passing @file
> with NULL.
> 
> Thanks,
> 
> >
> > Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> > ---
> >  fs/f2fs/data.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c index 95f30f0..afa12f1
> > 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -1333,9 +1333,11 @@ static int f2fs_read_data_pages(struct file *file,
> >  			struct address_space *mapping,
> >  			struct list_head *pages, unsigned nr_pages)  {
> > -	struct inode *inode = file->f_mapping->host;
> > +	struct inode *inode = mapping->host;
> >  	struct page *page = list_last_entry(pages, struct page, lru);
> >
> > +	if (likely(file != NULL))
> > +		BUG_ON(file->f_mapping != mapping);
> >  	trace_f2fs_readpages(inode, page, nr_pages);
> >
> >  	/* If the file has inline data, skip readpages */
> >
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Re: [PATCH] f2fs: allow readpages with NULL file pointer

[Chao Yu]

On 2017/9/21 13:00, gaoxiang (P) wrote:
> Keep in line with the other Linux file system implementations
> since page_cache_sync_readahead supports NULL file pointer,
> and thus we can readahead data by f2fs itself without file opening
> (something like the btrfs behavior).

Let's keep what it is until there is an example doing readahead passing @file
with NULL.

Thanks,

> 
> Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
> ---
>  fs/f2fs/data.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 95f30f0..afa12f1 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1333,9 +1333,11 @@ static int f2fs_read_data_pages(struct file *file,
>  			struct address_space *mapping,
>  			struct list_head *pages, unsigned nr_pages)
>  {
> -	struct inode *inode = file->f_mapping->host;
> +	struct inode *inode = mapping->host;
>  	struct page *page = list_last_entry(pages, struct page, lru);
>  
> +	if (likely(file != NULL))
> +		BUG_ON(file->f_mapping != mapping);
>  	trace_f2fs_readpages(inode, page, nr_pages);
>  
>  	/* If the file has inline data, skip readpages */
> 

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

[PATCH] f2fs: allow readpages with NULL file pointer

[gaoxiang (P)]

Keep in line with the other Linux file system implementations
since page_cache_sync_readahead supports NULL file pointer,
and thus we can readahead data by f2fs itself without file opening
(something like the btrfs behavior).

Signed-off-by: Gao Xiang <gaoxiang25@huawei.com>
---
 fs/f2fs/data.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 95f30f0..afa12f1 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -1333,9 +1333,11 @@ static int f2fs_read_data_pages(struct file *file,
 			struct address_space *mapping,
 			struct list_head *pages, unsigned nr_pages)
 {
-	struct inode *inode = file->f_mapping->host;
+	struct inode *inode = mapping->host;
 	struct page *page = list_last_entry(pages, struct page, lru);
 
+	if (likely(file != NULL))
+		BUG_ON(file->f_mapping != mapping);
 	trace_f2fs_readpages(inode, page, nr_pages);
 
 	/* If the file has inline data, skip readpages */
-- 
2.1.4


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot