| Message ID | 20240709191028.2329-5-wen.gang.wang@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | introduce defrag to xfs_spaceman | expand |
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? > + 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...) Also would you rather SIGINT merely terminate the spaceman process? I think the file locks drop on termination, right? --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) > >
> 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. > 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 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)
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) > >
> On Jul 15, 2024, at 3:56 PM, Darrick J. Wong <djwong@kernel.org> wrote: > > 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 Yep, will remove the print. Thanks, Wengang
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; + printf("Please wait until current segment is defragmented\n"); +}; + /* * 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:
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(-)