Btrfs devel - Feb 2012 - [PATCH] Btrfs: return EUCLEAN rather than ENXIO once internal error has occurred for SEEK_DATA/SEEK_HOLE inquiry

By referring to http://linux.die.net/man/2/lseek, return ENXIO only
when "offset beyond EOF" for either SEEK_DATA or SEEK_HOLE inquiry.
But we return it in case of internal issue too if btrfs_get_extent_fiemap()
failed
due to other issues.  This will confuse the user applications to be expecting
ENXIO when
trying to find a specific data or hole location once it has occurred.

Thanks Dave for pointing that out in XFS thread.

This patch fix it to return EUCLEAN, or maybe another particular errno is more
reasonable in Btrfs to indicate this fatal error?

Thanks,
-Jeff


Cc: david@fromorbit.com
Signed-off-by: Jie Liu <jeff.liu@oracle.com>

---
 fs/btrfs/file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 97fbe93..6693040 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -1761,7 +1761,7 @@ static int find_desired_extent(struct inode *inode, loff_t
*offset, int origin)
 						     start - root->sectorsize,
 						     root->sectorsize, 0);
 		if (IS_ERR(em)) {
-			ret = -ENXIO;
+			ret = -EUCLEAN;
 			goto out;
 		}
 		last_end = em->start + em->len;
@@ -1773,7 +1773,7 @@ static int find_desired_extent(struct inode *inode, loff_t
*offset, int origin)
 	while (1) {
 		em = btrfs_get_extent_fiemap(inode, NULL, 0, start, len, 0);
 		if (IS_ERR(em)) {
-			ret = -ENXIO;
+			ret = -EUCLEAN;
 			break;
 		}
 
-- 
1.7.9
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 02/09/2012 11:46 AM, Jeff Liu wrote:
> By referring to http://linux.die.net/man/2/lseek, return ENXIO only
> when "offset beyond EOF" for either SEEK_DATA or SEEK_HOLE
inquiry.
> But we return it in case of internal issue too if btrfs_get_extent_fiemap()
failed
> due to other issues.  This will confuse the user applications to be
expecting ENXIO when
> trying to find a specific data or hole location once it has occurred.
> 
> Thanks Dave for pointing that out in XFS thread.
> 
> This patch fix it to return EUCLEAN, or maybe another particular errno is
more reasonable in Btrfs to indicate this fatal error?
Or maybe just return the error that was happened at internal routine, to
give user more accurate error info, which is better?

Thanks,
-Jeff
> 
> Thanks,
> -Jeff
> 
> 
> Cc: david@fromorbit.com
> Signed-off-by: Jie Liu <jeff.liu@oracle.com>
> 
> ---
>  fs/btrfs/file.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index 97fbe93..6693040 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -1761,7 +1761,7 @@ static int find_desired_extent(struct inode *inode,
loff_t *offset, int origin)
>  						     start - root->sectorsize,
>  						     root->sectorsize, 0);
>  		if (IS_ERR(em)) {
> -			ret = -ENXIO;
> +			ret = -EUCLEAN;
>  			goto out;
>  		}
>  		last_end = em->start + em->len;
> @@ -1773,7 +1773,7 @@ static int find_desired_extent(struct inode *inode,
loff_t *offset, int origin)
>  	while (1) {
>  		em = btrfs_get_extent_fiemap(inode, NULL, 0, start, len, 0);
>  		if (IS_ERR(em)) {
> -			ret = -ENXIO;
> +			ret = -EUCLEAN;
>  			break;
>  		}
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 09, 2012 at 12:08:47PM +0800, Jeff Liu
wrote:
> On 02/09/2012 11:46 AM, Jeff Liu wrote:
> 
> > By referring to http://linux.die.net/man/2/lseek, return ENXIO only
> > when "offset beyond EOF" for either SEEK_DATA or SEEK_HOLE
inquiry.
> > But we return it in case of internal issue too if
btrfs_get_extent_fiemap() failed
> > due to other issues.  This will confuse the user applications to be
expecting ENXIO when
> > trying to find a specific data or hole location once it has occurred.
> > 
> > Thanks Dave for pointing that out in XFS thread.
> > 
> > This patch fix it to return EUCLEAN, or maybe another particular errno
is more reasonable in Btrfs to indicate this fatal error?
> 
> Or maybe just return the error that was happened at internal routine, to
> give user more accurate error info, which is better?
Return the internal error unchanged - a failure to read the extent
list (EIO) is different to a corruption detected in the extent
map read from disk (EUCLEAN). Having a user report the appropriate
error makes our life much simpler when it comes to trying to
understand their problem....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 02/09/2012 12:51 PM, Dave Chinner wrote:
> On Thu, Feb 09, 2012 at 12:08:47PM +0800, Jeff Liu wrote:
>> On 02/09/2012 11:46 AM, Jeff Liu wrote:
>>
>>> By referring to http://linux.die.net/man/2/lseek, return ENXIO only
>>> when "offset beyond EOF" for either SEEK_DATA or
SEEK_HOLE inquiry.
>>> But we return it in case of internal issue too if
btrfs_get_extent_fiemap() failed
>>> due to other issues.  This will confuse the user applications to be
expecting ENXIO when
>>> trying to find a specific data or hole location once it has
occurred.
>>>
>>> Thanks Dave for pointing that out in XFS thread.
>>>
>>> This patch fix it to return EUCLEAN, or maybe another particular
errno is more reasonable in Btrfs to indicate this fatal error?
>>
>> Or maybe just return the error that was happened at internal routine,
to
>> give user more accurate error info, which is better?
> 
> Return the internal error unchanged - a failure to read the extent
> list (EIO) is different to a corruption detected in the extent
> map read from disk (EUCLEAN). Having a user report the appropriate
> error makes our life much simpler when it comes to trying to
> understand their problem....
Definitely. I will repost this patch later.

Thanks,
-Jeff
> 
> Cheers,
> 
> Dave.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html