From: KOSAKI Motohiro <[email protected]>
Eric Wong reported his test suite was fail when /tmp is tmpfs.
https://lkml.org/lkml/2012/2/24/479
Current,input check of POSIX_FADV_WILLNEED has two problems.
1) require a_ops->readpage.
But in fact, force_page_cache_readahead() only require
a target filesystem has either ->readpage or ->readpages.
2) return -EINVAL when filesystem don't have ->readpage.
But, posix says, it should be retrieved a hint. Thus fadvise()
should return 0 if filesystem has no optimization way.
Especially, userland application don't know a filesystem type
of TMPDIR directory as Eric pointed out. Then, userland can't
avoid this error. We shouldn't encourage to ignore syscall
return value.
Thus, this patch change a return value to 0 when filesytem don't
support readahead.
Cc: [email protected]
Cc: Hugh Dickins <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Hillf Danton <[email protected]>
Signed-off-by: Eric Wong <[email protected]>
Tested-by: Eric Wong <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
---
mm/fadvise.c | 18 +++++++-----------
1 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/mm/fadvise.c b/mm/fadvise.c
index 469491e..33e6baf 100644
--- a/mm/fadvise.c
+++ b/mm/fadvise.c
@@ -93,11 +93,6 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
spin_unlock(&file->f_lock);
break;
case POSIX_FADV_WILLNEED:
- if (!mapping->a_ops->readpage) {
- ret = -EINVAL;
- break;
- }
-
/* First and last PARTIAL page! */
start_index = offset >> PAGE_CACHE_SHIFT;
end_index = endbyte >> PAGE_CACHE_SHIFT;
@@ -106,12 +101,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
nrpages = end_index - start_index + 1;
if (!nrpages)
nrpages = ~0UL;
-
- ret = force_page_cache_readahead(mapping, file,
- start_index,
- nrpages);
- if (ret > 0)
- ret = 0;
+
+ /*
+ * Ignore return value because fadvise() shall return
+ * success even if filesystem can't retrieve a hint,
+ */
+ force_page_cache_readahead(mapping, file, start_index,
+ nrpages);
break;
case POSIX_FADV_NOREUSE:
break;
--
1.7.1
On Fri, Jun 15, 2012 at 4:36 PM, <[email protected]> wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> Eric Wong reported his test suite was fail when /tmp is tmpfs.
>
> https://lkml.org/lkml/2012/2/24/479
>
> Current,input check of POSIX_FADV_WILLNEED has two problems.
>
> 1) require a_ops->readpage.
> ? But in fact, force_page_cache_readahead() only require
> ? a target filesystem has either ->readpage or ->readpages.
> 2) return -EINVAL when filesystem don't have ->readpage.
> ? But, posix says, it should be retrieved a hint. Thus fadvise()
> ? should return 0 if filesystem has no optimization way.
> ? Especially, userland application don't know a filesystem type
> ? of TMPDIR directory as Eric pointed out. Then, userland can't
> ? avoid this error. We shouldn't encourage to ignore syscall
> ? return value.
>
> Thus, this patch change a return value to 0 when filesytem don't
> support readahead.
>
> Cc: [email protected]
> Cc: Hugh Dickins <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Hillf Danton <[email protected]>
> Signed-off-by: Eric Wong <[email protected]>
> Tested-by: Eric Wong <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
no objection?
On 06/16/2012 04:36 AM, [email protected] wrote:
> From: KOSAKI Motohiro <[email protected]>
>
> Eric Wong reported his test suite was fail when /tmp is tmpfs.
>
> https://lkml.org/lkml/2012/2/24/479
>
> Current,input check of POSIX_FADV_WILLNEED has two problems.
>
> 1) require a_ops->readpage.
> But in fact, force_page_cache_readahead() only require
> a target filesystem has either ->readpage or ->readpages.
> 2) return -EINVAL when filesystem don't have ->readpage.
> But, posix says, it should be retrieved a hint. Thus fadvise()
> should return 0 if filesystem has no optimization way.
> Especially, userland application don't know a filesystem type
> of TMPDIR directory as Eric pointed out. Then, userland can't
> avoid this error. We shouldn't encourage to ignore syscall
> return value.
>
> Thus, this patch change a return value to 0 when filesytem don't
> support readahead.
>
> Cc: [email protected]
> Cc: Hugh Dickins <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Hillf Danton <[email protected]>
> Signed-off-by: Eric Wong <[email protected]>
> Tested-by: Eric Wong <[email protected]>
> Signed-off-by: KOSAKI Motohiro <[email protected]>
> ---
> mm/fadvise.c | 18 +++++++-----------
> 1 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/mm/fadvise.c b/mm/fadvise.c
> index 469491e..33e6baf 100644
> --- a/mm/fadvise.c
> +++ b/mm/fadvise.c
> @@ -93,11 +93,6 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
> spin_unlock(&file->f_lock);
> break;
> case POSIX_FADV_WILLNEED:
> - if (!mapping->a_ops->readpage) {
> - ret = -EINVAL;
> - break;
> - }
Why not check both readpage and readpages, if they are not here,
just beak and no following force_page_cache_readahead ?
Thanks,
Wanlong Gao
> -
> /* First and last PARTIAL page! */
> start_index = offset >> PAGE_CACHE_SHIFT;
> end_index = endbyte >> PAGE_CACHE_SHIFT;
> @@ -106,12 +101,13 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
> nrpages = end_index - start_index + 1;
> if (!nrpages)
> nrpages = ~0UL;
> -
> - ret = force_page_cache_readahead(mapping, file,
> - start_index,
> - nrpages);
> - if (ret > 0)
> - ret = 0;
> +
> + /*
> + * Ignore return value because fadvise() shall return
> + * success even if filesystem can't retrieve a hint,
> + */
> + force_page_cache_readahead(mapping, file, start_index,
> + nrpages);
> break;
> case POSIX_FADV_NOREUSE:
> break;
>
(6/20/12 2:31 AM), Wanlong Gao wrote:
> On 06/16/2012 04:36 AM, [email protected] wrote:
>> From: KOSAKI Motohiro <[email protected]>
>>
>> Eric Wong reported his test suite was fail when /tmp is tmpfs.
>>
>> https://lkml.org/lkml/2012/2/24/479
>>
>> Current,input check of POSIX_FADV_WILLNEED has two problems.
>>
>> 1) require a_ops->readpage.
>> But in fact, force_page_cache_readahead() only require
>> a target filesystem has either ->readpage or ->readpages.
>> 2) return -EINVAL when filesystem don't have ->readpage.
>> But, posix says, it should be retrieved a hint. Thus fadvise()
>> should return 0 if filesystem has no optimization way.
>> Especially, userland application don't know a filesystem type
>> of TMPDIR directory as Eric pointed out. Then, userland can't
>> avoid this error. We shouldn't encourage to ignore syscall
>> return value.
>>
>> Thus, this patch change a return value to 0 when filesytem don't
>> support readahead.
>>
>> Cc: [email protected]
>> Cc: Hugh Dickins <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>> Cc: Hillf Danton <[email protected]>
>> Signed-off-by: Eric Wong <[email protected]>
>> Tested-by: Eric Wong <[email protected]>
>> Signed-off-by: KOSAKI Motohiro <[email protected]>
>> ---
>> mm/fadvise.c | 18 +++++++-----------
>> 1 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/fadvise.c b/mm/fadvise.c
>> index 469491e..33e6baf 100644
>> --- a/mm/fadvise.c
>> +++ b/mm/fadvise.c
>> @@ -93,11 +93,6 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
>> spin_unlock(&file->f_lock);
>> break;
>> case POSIX_FADV_WILLNEED:
>> - if (!mapping->a_ops->readpage) {
>> - ret = -EINVAL;
>> - break;
>> - }
>
> Why not check both readpage and readpages, if they are not here,
> just beak and no following force_page_cache_readahead ?
They are checked in force_page_cache_readahead.
On 06/20/2012 02:33 PM, KOSAKI Motohiro wrote:
> (6/20/12 2:31 AM), Wanlong Gao wrote:
>> On 06/16/2012 04:36 AM, [email protected] wrote:
>>> From: KOSAKI Motohiro <[email protected]>
>>>
>>> Eric Wong reported his test suite was fail when /tmp is tmpfs.
>>>
>>> https://lkml.org/lkml/2012/2/24/479
>>>
>>> Current,input check of POSIX_FADV_WILLNEED has two problems.
>>>
>>> 1) require a_ops->readpage.
>>> But in fact, force_page_cache_readahead() only require
>>> a target filesystem has either ->readpage or ->readpages.
>>> 2) return -EINVAL when filesystem don't have ->readpage.
>>> But, posix says, it should be retrieved a hint. Thus fadvise()
>>> should return 0 if filesystem has no optimization way.
>>> Especially, userland application don't know a filesystem type
>>> of TMPDIR directory as Eric pointed out. Then, userland can't
>>> avoid this error. We shouldn't encourage to ignore syscall
>>> return value.
>>>
>>> Thus, this patch change a return value to 0 when filesytem don't
>>> support readahead.
>>>
>>> Cc: [email protected]
>>> Cc: Hugh Dickins <[email protected]>
>>> Cc: Andrew Morton <[email protected]>
>>> Cc: Hillf Danton <[email protected]>
>>> Signed-off-by: Eric Wong <[email protected]>
>>> Tested-by: Eric Wong <[email protected]>
>>> Signed-off-by: KOSAKI Motohiro <[email protected]>
>>> ---
>>> mm/fadvise.c | 18 +++++++-----------
>>> 1 files changed, 7 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/mm/fadvise.c b/mm/fadvise.c
>>> index 469491e..33e6baf 100644
>>> --- a/mm/fadvise.c
>>> +++ b/mm/fadvise.c
>>> @@ -93,11 +93,6 @@ SYSCALL_DEFINE(fadvise64_64)(int fd, loff_t offset, loff_t len, int advice)
>>> spin_unlock(&file->f_lock);
>>> break;
>>> case POSIX_FADV_WILLNEED:
>>> - if (!mapping->a_ops->readpage) {
>>> - ret = -EINVAL;
>>> - break;
>>> - }
>>
>> Why not check both readpage and readpages, if they are not here,
>> just beak and no following force_page_cache_readahead ?
>
> They are checked in force_page_cache_readahead.
I see, thank you.
Reviewed-by: Wanlong Gao <[email protected]>
>
>