[PATCH] xfs: Allow user to kill fstrim process

[PATCH] xfs: Allow user to kill fstrim process

[Lukas Czerner]

fstrim can take really long time on big, slow device or on file system
with a lots of allocation groups. Currently there is no way for the user
to cancell the operation. This patch makes it possible for the user to
kill fstrim pocess by adding the check for fatal_signal_pending() in
xfs_trim_extents().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
---
 fs/xfs/xfs_discard.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index d796ffa..6a05d27 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -132,6 +132,11 @@ xfs_trim_extents(
 		error = xfs_btree_decrement(cur, 0, &i);
 		if (error)
 			goto out_del_cursor;
+
+		if (fatal_signal_pending(current)) {
+			error = -ERESTARTSYS;
+			goto out_del_cursor;
+		}
 	}
 
 out_del_cursor:
@@ -196,8 +201,11 @@ xfs_ioc_trim(
 	for (agno = start_agno; agno <= end_agno; agno++) {
 		error = xfs_trim_extents(mp, agno, start, end, minlen,
 					  &blocks_trimmed);
-		if (error)
+		if (error) {
 			last_error = error;
+			if (error == -ERESTARTSYS)
+				break;
+		}
 	}
 
 	if (last_error)
-- 
2.7.4

Re: [PATCH] xfs: Allow user to kill fstrim process

[Eric Sandeen]

On 4/27/17 3:39 AM, Lukas Czerner wrote:
> fstrim can take really long time on big, slow device or on file system
> with a lots of allocation groups. Currently there is no way for the user
> to cancell the operation. This patch makes it possible for the user to
> kill fstrim pocess by adding the check for fatal_signal_pending() in
> xfs_trim_extents().

With this patch, it seems that if it is killed, then nothing is copied
back to the user about the number of blocks trimmed before the kill.

Is that intentional?

I think it could be done in a way that the progress until the kill
does get reported, and that might be more useful?

Thanks,
-Eric

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> ---
>  fs/xfs/xfs_discard.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index d796ffa..6a05d27 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -132,6 +132,11 @@ xfs_trim_extents(
>  		error = xfs_btree_decrement(cur, 0, &i);
>  		if (error)
>  			goto out_del_cursor;
> +
> +		if (fatal_signal_pending(current)) {
> +			error = -ERESTARTSYS;
> +			goto out_del_cursor;
> +		}
>  	}
>  
>  out_del_cursor:
> @@ -196,8 +201,11 @@ xfs_ioc_trim(
>  	for (agno = start_agno; agno <= end_agno; agno++) {
>  		error = xfs_trim_extents(mp, agno, start, end, minlen,
>  					  &blocks_trimmed);
> -		if (error)
> +		if (error) {
>  			last_error = error;
> +			if (error == -ERESTARTSYS)
> +				break;
> +		}
>  	}
>  
>  	if (last_error)
> 

Re: [PATCH] xfs: Allow user to kill fstrim process

[Darrick J. Wong]

On Thu, Apr 27, 2017 at 09:34:04AM -0500, Eric Sandeen wrote:
> On 4/27/17 3:39 AM, Lukas Czerner wrote:
> > fstrim can take really long time on big, slow device or on file system
> > with a lots of allocation groups. Currently there is no way for the user
> > to cancell the operation. This patch makes it possible for the user to
> > kill fstrim pocess by adding the check for fatal_signal_pending() in
> > xfs_trim_extents().
> 
> With this patch, it seems that if it is killed, then nothing is copied
> back to the user about the number of blocks trimmed before the kill.
> 
> Is that intentional?
> 
> I think it could be done in a way that the progress until the kill
> does get reported, and that might be more useful?

AFAICT ext4 doesn't report anything if somoene ERESTARTSYS's it...

> 
> Thanks,
> -Eric
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
> > ---
> >  fs/xfs/xfs_discard.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> > index d796ffa..6a05d27 100644
> > --- a/fs/xfs/xfs_discard.c
> > +++ b/fs/xfs/xfs_discard.c
> > @@ -132,6 +132,11 @@ xfs_trim_extents(
> >  		error = xfs_btree_decrement(cur, 0, &i);
> >  		if (error)
> >  			goto out_del_cursor;
> > +
> > +		if (fatal_signal_pending(current)) {

...but the process isn't going to be around to care anyway, right?

--D

> > +			error = -ERESTARTSYS;
> > +			goto out_del_cursor;
> > +		}
> >  	}
> >  
> >  out_del_cursor:
> > @@ -196,8 +201,11 @@ xfs_ioc_trim(
> >  	for (agno = start_agno; agno <= end_agno; agno++) {
> >  		error = xfs_trim_extents(mp, agno, start, end, minlen,
> >  					  &blocks_trimmed);
> > -		if (error)
> > +		if (error) {
> >  			last_error = error;
> > +			if (error == -ERESTARTSYS)
> > +				break;
> > +		}
> >  	}
> >  
> >  	if (last_error)
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfs: Allow user to kill fstrim process

[Eric Sandeen]

On 4/27/17 10:38 AM, Darrick J. Wong wrote:
> On Thu, Apr 27, 2017 at 09:34:04AM -0500, Eric Sandeen wrote:
>> On 4/27/17 3:39 AM, Lukas Czerner wrote:
>>> fstrim can take really long time on big, slow device or on file system
>>> with a lots of allocation groups. Currently there is no way for the user
>>> to cancell the operation. This patch makes it possible for the user to
>>> kill fstrim pocess by adding the check for fatal_signal_pending() in
>>> xfs_trim_extents().
>>
>> With this patch, it seems that if it is killed, then nothing is copied
>> back to the user about the number of blocks trimmed before the kill.
>>
>> Is that intentional?
>>
>> I think it could be done in a way that the progress until the kill
>> does get reported, and that might be more useful?
> 
> AFAICT ext4 doesn't report anything if somoene ERESTARTSYS's it...

Well we aren't ext4, are we?  ;)

actually, it seems that it may report "count" as ERESTARTSYS in:

        ext4_debug("trimmed %d blocks in the group %d\n",
                count, group);

But yeah, although it does update the &range values, they don't
get copied out on any error.

Anyway, ok, ext4 doesn't.  Is there a reason that we shouldn't?

Thanks,
-Eric

>>
>> Thanks,
>> -Eric
>>
>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>>> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>
>>> ---
>>>  fs/xfs/xfs_discard.c | 10 +++++++++-
>>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
>>> index d796ffa..6a05d27 100644
>>> --- a/fs/xfs/xfs_discard.c
>>> +++ b/fs/xfs/xfs_discard.c
>>> @@ -132,6 +132,11 @@ xfs_trim_extents(
>>>  		error = xfs_btree_decrement(cur, 0, &i);
>>>  		if (error)
>>>  			goto out_del_cursor;
>>> +
>>> +		if (fatal_signal_pending(current)) {
> 
> ...but the process isn't going to be around to care anyway, right?
> 
> --D
> 
>>> +			error = -ERESTARTSYS;
>>> +			goto out_del_cursor;
>>> +		}
>>>  	}
>>>  
>>>  out_del_cursor:
>>> @@ -196,8 +201,11 @@ xfs_ioc_trim(
>>>  	for (agno = start_agno; agno <= end_agno; agno++) {
>>>  		error = xfs_trim_extents(mp, agno, start, end, minlen,
>>>  					  &blocks_trimmed);
>>> -		if (error)
>>> +		if (error) {
>>>  			last_error = error;
>>> +			if (error == -ERESTARTSYS)
>>> +				break;
>>> +		}
>>>  	}
>>>  
>>>  	if (last_error)
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] xfs: Allow user to kill fstrim process

[Eric Sandeen]

On 4/27/17 10:46 AM, Eric Sandeen wrote:
> On 4/27/17 10:38 AM, Darrick J. Wong wrote:
>> On Thu, Apr 27, 2017 at 09:34:04AM -0500, Eric Sandeen wrote:
>>> On 4/27/17 3:39 AM, Lukas Czerner wrote:
>>>> fstrim can take really long time on big, slow device or on file system
>>>> with a lots of allocation groups. Currently there is no way for the user
>>>> to cancell the operation. This patch makes it possible for the user to
>>>> kill fstrim pocess by adding the check for fatal_signal_pending() in
>>>> xfs_trim_extents().
>>>
>>> With this patch, it seems that if it is killed, then nothing is copied
>>> back to the user about the number of blocks trimmed before the kill.
>>>
>>> Is that intentional?
>>>
>>> I think it could be done in a way that the progress until the kill
>>> does get reported, and that might be more useful?
>>
>> AFAICT ext4 doesn't report anything if somoene ERESTARTSYS's it...
> 
> Well we aren't ext4, are we?  ;)
> 
> actually, it seems that it may report "count" as ERESTARTSYS in:
> 
>         ext4_debug("trimmed %d blocks in the group %d\n",
>                 count, group);
> 
> But yeah, although it does update the &range values, they don't
> get copied out on any error.
> 
> Anyway, ok, ext4 doesn't.  Is there a reason that we shouldn't?

Ok I am probably misunderstanding how things work, maybe it's
not actually possible (he says after a little experimentation.)
Feel free to ignore me on this one.  :)

-Eric
 

Re: [PATCH] xfs: Allow user to kill fstrim process

[Lukas Czerner]

On Thu, Apr 27, 2017 at 10:50:02AM -0500, Eric Sandeen wrote:
> On 4/27/17 10:46 AM, Eric Sandeen wrote:
> > On 4/27/17 10:38 AM, Darrick J. Wong wrote:
> >> On Thu, Apr 27, 2017 at 09:34:04AM -0500, Eric Sandeen wrote:
> >>> On 4/27/17 3:39 AM, Lukas Czerner wrote:
> >>>> fstrim can take really long time on big, slow device or on file system
> >>>> with a lots of allocation groups. Currently there is no way for the user
> >>>> to cancell the operation. This patch makes it possible for the user to
> >>>> kill fstrim pocess by adding the check for fatal_signal_pending() in
> >>>> xfs_trim_extents().
> >>>
> >>> With this patch, it seems that if it is killed, then nothing is copied
> >>> back to the user about the number of blocks trimmed before the kill.
> >>>
> >>> Is that intentional?
> >>>
> >>> I think it could be done in a way that the progress until the kill
> >>> does get reported, and that might be more useful?
> >>
> >> AFAICT ext4 doesn't report anything if somoene ERESTARTSYS's it...
> > 
> > Well we aren't ext4, are we?  ;)
> > 
> > actually, it seems that it may report "count" as ERESTARTSYS in:
> > 
> >         ext4_debug("trimmed %d blocks in the group %d\n",
> >                 count, group);
> > 
> > But yeah, although it does update the &range values, they don't
> > get copied out on any error.
> > 
> > Anyway, ok, ext4 doesn't.  Is there a reason that we shouldn't?
> 
> Ok I am probably misunderstanding how things work, maybe it's
> not actually possible (he says after a little experimentation.)
> Feel free to ignore me on this one.  :)
> 
> -Eric

Hi Eric,

I agree with Darrick, at the moment fstrim is not sticikng around to
care, I am not even sure if it can stick around to care.

Thanks!
-Lukas

Re: [PATCH] xfs: Allow user to kill fstrim process

[Eric Sandeen]

On 4/27/17 3:39 AM, Lukas Czerner wrote:
> fstrim can take really long time on big, slow device or on file system
> with a lots of allocation groups. Currently there is no way for the user
> to cancell the operation. This patch makes it possible for the user to
> kill fstrim pocess by adding the check for fatal_signal_pending() in
> xfs_trim_extents().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>

Sorry for the detour while I got confused about signals.  ;)

This looks ok to me.  Thanks!

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/xfs/xfs_discard.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index d796ffa..6a05d27 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -132,6 +132,11 @@ xfs_trim_extents(
>  		error = xfs_btree_decrement(cur, 0, &i);
>  		if (error)
>  			goto out_del_cursor;
> +
> +		if (fatal_signal_pending(current)) {
> +			error = -ERESTARTSYS;
> +			goto out_del_cursor;
> +		}
>  	}
>  
>  out_del_cursor:
> @@ -196,8 +201,11 @@ xfs_ioc_trim(
>  	for (agno = start_agno; agno <= end_agno; agno++) {
>  		error = xfs_trim_extents(mp, agno, start, end, minlen,
>  					  &blocks_trimmed);
> -		if (error)
> +		if (error) {
>  			last_error = error;
> +			if (error == -ERESTARTSYS)
> +				break;
> +		}
>  	}
>  
>  	if (last_error)
> 

Re: [PATCH] xfs: Allow user to kill fstrim process

[Darrick J. Wong]

On Thu, Apr 27, 2017 at 10:39:28AM +0200, Lukas Czerner wrote:
> fstrim can take really long time on big, slow device or on file system
> with a lots of allocation groups. Currently there is no way for the user
> to cancell the operation. This patch makes it possible for the user to
> kill fstrim pocess by adding the check for fatal_signal_pending() in
> xfs_trim_extents().
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Zdenek Kabelac <zkabelac@redhat.com>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_discard.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
> index d796ffa..6a05d27 100644
> --- a/fs/xfs/xfs_discard.c
> +++ b/fs/xfs/xfs_discard.c
> @@ -132,6 +132,11 @@ xfs_trim_extents(
>  		error = xfs_btree_decrement(cur, 0, &i);
>  		if (error)
>  			goto out_del_cursor;
> +
> +		if (fatal_signal_pending(current)) {
> +			error = -ERESTARTSYS;
> +			goto out_del_cursor;
> +		}
>  	}
>  
>  out_del_cursor:
> @@ -196,8 +201,11 @@ xfs_ioc_trim(
>  	for (agno = start_agno; agno <= end_agno; agno++) {
>  		error = xfs_trim_extents(mp, agno, start, end, minlen,
>  					  &blocks_trimmed);
> -		if (error)
> +		if (error) {
>  			last_error = error;
> +			if (error == -ERESTARTSYS)
> +				break;
> +		}
>  	}
>  
>  	if (last_error)
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html