[RH7] ext4: Fix fiemap() with FIEMAP_FLAG_CACHE flag

Submitted by Kirill Tkhai on Aug. 11, 2020, 2:02 p.m.

Details

Message ID 159715453338.140764.155795136599509587.stgit@localhost.localdomain
State New
Series "ext4: Fix fiemap() with FIEMAP_FLAG_CACHE flag"
Headers show

Commit Message

Kirill Tkhai Aug. 11, 2020, 2:02 p.m.
This flag says to cache extents in extent tree before iterations over them,
but the combination of these two actions does not work.

ext4_ext_precache() populates cache, but extent iteration can't occur,
since fiemap_check_flags() does not know this flag.

So, the result of this flag is: 1)cache is populated, 2)-EBADR is returned
instead of extents array. This looks like a BUG in mainstream, which was
blindly fixed in commit bb5835edcdf8bf7 "ext4: add new ioctl EXT4_IOC_GET_ES_CACHE".

This patch makes the flag to work as expected.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/ext4/extents.c |    1 +
 1 file changed, 1 insertion(+)

Patch hide | download patch | download mbox

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 8d3e509f39b9..74a283d3ce36 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5250,6 +5250,7 @@  int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
 		error = ext4_ext_precache(inode);
 		if (error)
 			return error;
+		fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
 	}
 
 	/* fallback to generic here if not in extents fmt */

Comments

Vasily Averin Aug. 12, 2020, 6:09 a.m.
Kirill,

do you have bugID for this problem?

Thank you,
	Vasily Averin

On 8/11/20 5:02 PM, Kirill Tkhai wrote:
> This flag says to cache extents in extent tree before iterations over them,
> but the combination of these two actions does not work.
> 
> ext4_ext_precache() populates cache, but extent iteration can't occur,
> since fiemap_check_flags() does not know this flag.
> 
> So, the result of this flag is: 1)cache is populated, 2)-EBADR is returned
> instead of extents array. This looks like a BUG in mainstream, which was
> blindly fixed in commit bb5835edcdf8bf7 "ext4: add new ioctl EXT4_IOC_GET_ES_CACHE".
> 
> This patch makes the flag to work as expected.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> ---
>  fs/ext4/extents.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 8d3e509f39b9..74a283d3ce36 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5250,6 +5250,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		error = ext4_ext_precache(inode);
>  		if (error)
>  			return error;
> +		fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
>  	}
>  
>  	/* fallback to generic here if not in extents fmt */
> 
>
Konstantin Khorenko Aug. 12, 2020, 9:37 a.m.
On 08/11/2020 05:02 PM, Kirill Tkhai wrote:
> This flag says to cache extents in extent tree before iterations over them,
> but the combination of these two actions does not work.
>
> ext4_ext_precache() populates cache, but extent iteration can't occur,
> since fiemap_check_flags() does not know this flag.
>
> So, the result of this flag is: 1)cache is populated, 2)-EBADR is returned
> instead of extents array. This looks like a BUG in mainstream, which was
> blindly fixed in commit bb5835edcdf8bf7 "ext4: add new ioctl EXT4_IOC_GET_ES_CACHE".
>
> This patch makes the flag to work as expected.
>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

Acked-by: Konstantin Khorenko <khorenko@virtuozzo.com>

Note: Kirill applied the fiemap behavior from ms kernel,
but there is one question left:
fiemap doc says that "If the kernel is compatible with all flags passed,
the contents of fm_flags will be unmodified."

But after this patch this statement becomes invalid.
(ms behavior is the same)

So need to fix mainstream: either documentation or the code,
but this is another story.

> ---
>  fs/ext4/extents.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 8d3e509f39b9..74a283d3ce36 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5250,6 +5250,7 @@ int ext4_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo,
>  		error = ext4_ext_precache(inode);
>  		if (error)
>  			return error;
> +		fieinfo->fi_flags &= ~FIEMAP_FLAG_CACHE;
>  	}
>
>  	/* fallback to generic here if not in extents fmt */
>
>
> .
>