[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Andrew Price]

The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to
fs block size") fixes was a little confusing as the user saw
"Input/output error" which masked the -EINVAL that sb_issue_discard()
returned.

sb_issue_discard() can fail for various reasons, so we should return its
return value from gfs2_rgrp_send_discards() to avoid all errors being
reported as IO errors.

This improves error reporting for FITRIM and makes no difference to the
-o discard code path because the return value from
gfs2_rgrp_send_discards() gets thrown away in that case (and the option
switches off). Presumably that's why it was ok to just return -EIO in
the past, before FITRIM was implemented.

Tested with xfstests.

Signed-off-by: Andrew Price <anprice@redhat.com>
---

I don't see this in for-next yet so I've updated the commit log to include more
details.

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

diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 801ad9f4f2be..886343cc05ab 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1315,7 +1315,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 	u64 blk;
 	sector_t start = 0;
 	sector_t nr_blks = 0;
-	int rv;
+	int rv = -EIO;
 	unsigned int x;
 	u32 trimmed = 0;
 	u8 diff;
@@ -1371,7 +1371,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
 	if (sdp->sd_args.ar_discard)
 		fs_warn(sdp, "error %d on discard request, turning discards off for this filesystem\n", rv);
 	sdp->sd_args.ar_discard = 0;
-	return -EIO;
+	return rv;
 }
 
 /**
-- 
2.35.1

[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Andrew Price]

On 05/04/2022 13:08, Andrew Price wrote:
> The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to
> fs block size") fixes was a little confusing as the user saw
> "Input/output error" which masked the -EINVAL that sb_issue_discard()
> returned.
> 
> sb_issue_discard() can fail for various reasons, so we should return its
> return value from gfs2_rgrp_send_discards() to avoid all errors being
> reported as IO errors.
> 
> This improves error reporting for FITRIM and makes no difference to the
> -o discard code path because the return value from
> gfs2_rgrp_send_discards() gets thrown away in that case (and the option
> switches off). Presumably that's why it was ok to just return -EIO in
> the past, before FITRIM was implemented.
> 
> Tested with xfstests.

Can I get a thumbs-up or thumbs-down for this patch? It's pretty 
straightforward. I just don't want it to get forgotten about.

Cheers,
Andy

> Signed-off-by: Andrew Price <anprice@redhat.com>
> ---
> 
> I don't see this in for-next yet so I've updated the commit log to include more
> details.
> 
>   fs/gfs2/rgrp.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
> index 801ad9f4f2be..886343cc05ab 100644
> --- a/fs/gfs2/rgrp.c
> +++ b/fs/gfs2/rgrp.c
> @@ -1315,7 +1315,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
>   	u64 blk;
>   	sector_t start = 0;
>   	sector_t nr_blks = 0;
> -	int rv;
> +	int rv = -EIO;
>   	unsigned int x;
>   	u32 trimmed = 0;
>   	u8 diff;
> @@ -1371,7 +1371,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd *sdp, u64 offset,
>   	if (sdp->sd_args.ar_discard)
>   		fs_warn(sdp, "error %d on discard request, turning discards off for this filesystem\n", rv);
>   	sdp->sd_args.ar_discard = 0;
> -	return -EIO;
> +	return rv;
>   }
>   
>   /**

[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Andrew Price]

On 19/04/2022 16:49, Andrew Price wrote:
> On 05/04/2022 13:08, Andrew Price wrote:
>> The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to
>> fs block size") fixes was a little confusing as the user saw
>> "Input/output error" which masked the -EINVAL that sb_issue_discard()
>> returned.
>>
>> sb_issue_discard() can fail for various reasons, so we should return its
>> return value from gfs2_rgrp_send_discards() to avoid all errors being
>> reported as IO errors.
>>
>> This improves error reporting for FITRIM and makes no difference to the
>> -o discard code path because the return value from
>> gfs2_rgrp_send_discards() gets thrown away in that case (and the option
>> switches off). Presumably that's why it was ok to just return -EIO in
>> the past, before FITRIM was implemented.
>>
>> Tested with xfstests.
> 
> Can I get a thumbs-up or thumbs-down for this patch? It's pretty 
> straightforward. I just don't want it to get forgotten about.
> 
> Cheers,
> Andy

Please could you take this patch?

Cheers,
Andy

>> Signed-off-by: Andrew Price <anprice@redhat.com>
>> ---
>>
>> I don't see this in for-next yet so I've updated the commit log to 
>> include more
>> details.
>>
>> ? fs/gfs2/rgrp.c | 4 ++--
>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
>> index 801ad9f4f2be..886343cc05ab 100644
>> --- a/fs/gfs2/rgrp.c
>> +++ b/fs/gfs2/rgrp.c
>> @@ -1315,7 +1315,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd 
>> *sdp, u64 offset,
>> ????? u64 blk;
>> ????? sector_t start = 0;
>> ????? sector_t nr_blks = 0;
>> -??? int rv;
>> +??? int rv = -EIO;
>> ????? unsigned int x;
>> ????? u32 trimmed = 0;
>> ????? u8 diff;
>> @@ -1371,7 +1371,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd 
>> *sdp, u64 offset,
>> ????? if (sdp->sd_args.ar_discard)
>> ????????? fs_warn(sdp, "error %d on discard request, turning discards 
>> off for this filesystem\n", rv);
>> ????? sdp->sd_args.ar_discard = 0;
>> -??? return -EIO;
>> +??? return rv;
>> ? }
>> ? /**
> 

[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Andrew Price]

On 29/04/2022 15:08, Andrew Price wrote:
> On 19/04/2022 16:49, Andrew Price wrote:
>> On 05/04/2022 13:08, Andrew Price wrote:
>>> The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to
>>> fs block size") fixes was a little confusing as the user saw
>>> "Input/output error" which masked the -EINVAL that sb_issue_discard()
>>> returned.
>>>
>>> sb_issue_discard() can fail for various reasons, so we should return its
>>> return value from gfs2_rgrp_send_discards() to avoid all errors being
>>> reported as IO errors.
>>>
>>> This improves error reporting for FITRIM and makes no difference to the
>>> -o discard code path because the return value from
>>> gfs2_rgrp_send_discards() gets thrown away in that case (and the option
>>> switches off). Presumably that's why it was ok to just return -EIO in
>>> the past, before FITRIM was implemented.
>>>
>>> Tested with xfstests.
>>
>> Can I get a thumbs-up or thumbs-down for this patch? It's pretty 
>> straightforward. I just don't want it to get forgotten about.
>>
>> Cheers,
>> Andy
> 
> Please could you take this patch?
> 
> Cheers,
> Andy

For convenience, I've pushed this patch to gitlab:

   git fetch https://gitlab.com/andyprice/linux.git discard_errors
   git show FETCH_HEAD

Cheers,
Andy

>>> Signed-off-by: Andrew Price <anprice@redhat.com>
>>> ---
>>>
>>> I don't see this in for-next yet so I've updated the commit log to 
>>> include more
>>> details.
>>>
>>> ? fs/gfs2/rgrp.c | 4 ++--
>>> ? 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
>>> index 801ad9f4f2be..886343cc05ab 100644
>>> --- a/fs/gfs2/rgrp.c
>>> +++ b/fs/gfs2/rgrp.c
>>> @@ -1315,7 +1315,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd 
>>> *sdp, u64 offset,
>>> ????? u64 blk;
>>> ????? sector_t start = 0;
>>> ????? sector_t nr_blks = 0;
>>> -??? int rv;
>>> +??? int rv = -EIO;
>>> ????? unsigned int x;
>>> ????? u32 trimmed = 0;
>>> ????? u8 diff;
>>> @@ -1371,7 +1371,7 @@ int gfs2_rgrp_send_discards(struct gfs2_sbd 
>>> *sdp, u64 offset,
>>> ????? if (sdp->sd_args.ar_discard)
>>> ????????? fs_warn(sdp, "error %d on discard request, turning discards 
>>> off for this filesystem\n", rv);
>>> ????? sdp->sd_args.ar_discard = 0;
>>> -??? return -EIO;
>>> +??? return rv;
>>> ? }
>>> ? /**
>>
> 

[Cluster-devel] [PATCH RESEND] gfs2: Return more useful errors from gfs2_rgrp_send_discards()

[Andreas Gruenbacher]

On Mon, May 9, 2022 at 12:41 PM Andrew Price <anprice@redhat.com> wrote:
> On 29/04/2022 15:08, Andrew Price wrote:
> > On 19/04/2022 16:49, Andrew Price wrote:
> >> On 05/04/2022 13:08, Andrew Price wrote:
> >>> The bug that 27ca8273f ("gfs2: Make sure FITRIM minlen is rounded up to
> >>> fs block size") fixes was a little confusing as the user saw
> >>> "Input/output error" which masked the -EINVAL that sb_issue_discard()
> >>> returned.
> >>>
> >>> sb_issue_discard() can fail for various reasons, so we should return its
> >>> return value from gfs2_rgrp_send_discards() to avoid all errors being
> >>> reported as IO errors.
> >>>
> >>> This improves error reporting for FITRIM and makes no difference to the
> >>> -o discard code path because the return value from
> >>> gfs2_rgrp_send_discards() gets thrown away in that case (and the option
> >>> switches off). Presumably that's why it was ok to just return -EIO in
> >>> the past, before FITRIM was implemented.
> >>>
> >>> Tested with xfstests.
> >>
> >> Can I get a thumbs-up or thumbs-down for this patch? It's pretty
> >> straightforward. I just don't want it to get forgotten about.
> >>
> >> Cheers,
> >> Andy
> >
> > Please could you take this patch?
> >
> > Cheers,
> > Andy
>
> For convenience, I've pushed this patch to gitlab:
>
>    git fetch https://gitlab.com/andyprice/linux.git discard_errors
>    git show FETCH_HEAD

Pulled, thanks.

Andreas