Re: [PATCH 4/9] spaceman/defrag: ctrl-c handler

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Jul 11, 2024 at 10:58:02PM +0000, Wengang Wang wrote:
> 
> 
> > On Jul 9, 2024, at 2:08 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> > 
> > On Tue, Jul 09, 2024 at 12:10:23PM -0700, Wengang Wang wrote:
> >> Add this handler to break the defrag better, so it has
> >> 1. the stats reporting
> >> 2. remove the temporary file
> >> 
> >> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >> ---
> >> spaceman/defrag.c | 11 ++++++++++-
> >> 1 file changed, 10 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/spaceman/defrag.c b/spaceman/defrag.c
> >> index 9f11e36b..61e47a43 100644
> >> --- a/spaceman/defrag.c
> >> +++ b/spaceman/defrag.c
> >> @@ -297,6 +297,13 @@ get_time_delta_us(struct timeval *pre_time, struct timeval *cur_time)
> >> return us;
> >> }
> >> 
> >> +static volatile bool usedKilled = false;
> >> +void defrag_sigint_handler(int dummy)
> >> +{
> >> + usedKilled = true;
> > 
> > Not sure why some of these variables are camelCase and others not.
> > Or why this global variable doesn't have a g_ prefix like the others?
> > 
> 
> Yep, will change it to g_user_killed.
> 
> >> + printf("Please wait until current segment is defragmented\n");
> > 
> > Is it actually safe to call printf from a signal handler?  Handlers must
> > be very careful about what they call -- regreSSHion was a result of
> > openssh not getting this right.
> > 
> > (Granted spaceman isn't as critical...)
> > 
> 
> As the ioctl of UNSHARE takes time, so the process would really stop a while
> After user’s kil. The message is used as a quick response to user. It’s not actually
> Has any functionality. If it’s not safe, we can remove the message.

$ man signal-safety

> > Also would you rather SIGINT merely terminate the spaceman process?  I
> > think the file locks drop on termination, right?
> 
> Another purpose of the handler is that I want to show the stats like below even process is killed:
> 
> Pre-defrag 54699 extents detected, 0 are "unwritten",0 are "shared"
> Tried to defragment 54697 extents (939511808 bytes) in 57 segments
> Time stats(ms): max clone: 33, max unshare: 2254, max punch_hole: 286
> Post-defrag 12617 extents detected

Ah, ok.

> Thanks,
> Winging
> 
> > 
> > --D
> > 
> >> +};
> >> +
> >> /*
> >>  * defragment a file
> >>  * return 0 if successfully done, 1 otherwise
> >> @@ -345,6 +352,8 @@ defrag_xfs_defrag(char *file_path) {
> >> goto out;
> >> }
> >> 
> >> + signal(SIGINT, defrag_sigint_handler);
> >> +
> >> do {
> >> struct timeval t_clone, t_unshare, t_punch_hole;
> >> struct defrag_segment segment;
> >> @@ -434,7 +443,7 @@ defrag_xfs_defrag(char *file_path) {
> >> if (time_delta > max_punch_us)
> >> max_punch_us = time_delta;
> >> 
> >> - if (stop)
> >> + if (stop || usedKilled)
> >> break;
> >> } while (true);
> >> out:
> >> -- 
> >> 2.39.3 (Apple Git-146)
> 
>