Re: [PATCH 3/9] spaceman/defrag: defrag segments

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

On Fri, Jul 12, 2024 at 07:07:01PM +0000, Wengang Wang wrote:
> 
> 
> > On Jul 11, 2024, at 3:49 PM, Wengang Wang <wen.gang.wang@oracle.com> wrote:
> > 
> > 
> > 
> >> On Jul 9, 2024, at 2:57 PM, Darrick J. Wong <djwong@kernel.org> wrote:
> >> 
> >> On Tue, Jul 09, 2024 at 12:10:22PM -0700, Wengang Wang wrote:
> >>> For each segment, the following steps are done trying to defrag it:
> >>> 
> >>> 1. share the segment with a temporary file
> >>> 2. unshare the segment in the target file. kernel simulates Cow on the whole
> >>>  segment complete the unshare (defrag).
> >>> 3. release blocks from the tempoary file.
> >>> 
> >>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> >>> ---
> >>> spaceman/defrag.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
> >>> 1 file changed, 114 insertions(+)
> >>> 
> >>> diff --git a/spaceman/defrag.c b/spaceman/defrag.c
> >>> index 175cf461..9f11e36b 100644
> >>> --- a/spaceman/defrag.c
> >>> +++ b/spaceman/defrag.c

<snip>

> >>> @@ -322,6 +363,79 @@ defrag_xfs_defrag(char *file_path) {
> >>> ret = 1;
> >>> break;
> >>> }
> >>> +
> >>> + /* we are done if the segment contains only 1 extent */
> >>> + if (segment.ds_nr < 2)
> >>> + continue;
> >>> +
> >>> + /* to bytes */
> >>> + seg_off = segment.ds_offset * 512;
> >>> + seg_size = segment.ds_length * 512;
> >>> +
> >>> + clone.src_offset = seg_off;
> >>> + clone.src_length = seg_size;
> >>> + clone.dest_offset = seg_off;
> >>> +
> >>> + /* checks for EoF and fix up clone */
> >>> + stop = defrag_clone_eof(&clone);
> >>> + gettimeofday(&t_clone, NULL);
> >>> + ret = ioctl(scratch_fd, FICLONERANGE, &clone);
> >> 
> >> Hm, should the top-level defrag_f function check in the
> >> filetable[i].fsgeom structure that the fs supports reflink?
> > 
> > Yes, good to know.
> 
> It seems that xfs_fsop_geom doesn’t know about reflink?

XFS_FSOP_GEOM_FLAGS_REFLINK ?

--D

> Thanks,
> Wengang 
> 
> > 
> >> 
> >>> + if (ret != 0) {
> >>> + fprintf(stderr, "FICLONERANGE failed %s\n",
> >>> + strerror(errno));
> >> 
> >> Might be useful to include the file_path in the error message:
> >> 
> >> /opt/a: FICLONERANGE failed Software caused connection abort
> >> 
> >> (maybe also put a semicolon before the strerror message?)
> > 
> > OK.
> > 
> >> 
> >>> + break;
> >>> + }
> >>> +
> >>> + /* for time stats */
> >>> + time_delta = get_time_delta_us(&t_clone, &t_unshare);
> >>> + if (time_delta > max_clone_us)
> >>> + max_clone_us = time_delta;
> >>> +
> >>> + /* for defrag stats */
> >>> + nr_ext_defrag += segment.ds_nr;
> >>> +
> >>> + /*
> >>> +  * For the shared range to be unshared via a copy-on-write
> >>> +  * operation in the file to be defragged. This causes the
> >>> +  * file needing to be defragged to have new extents allocated
> >>> +  * and the data to be copied over and written out.
> >>> +  */
> >>> + ret = fallocate(defrag_fd, FALLOC_FL_UNSHARE_RANGE, seg_off,
> >>> + seg_size);
> >>> + if (ret != 0) {
> >>> + fprintf(stderr, "UNSHARE_RANGE failed %s\n",
> >>> + strerror(errno));
> >>> + break;
> >>> + }
> >>> +
> >>> + /* for time stats */
> >>> + time_delta = get_time_delta_us(&t_unshare, &t_punch_hole);
> >>> + if (time_delta > max_unshare_us)
> >>> + max_unshare_us = time_delta;
> >>> +
> >>> + /*
> >>> +  * Punch out the original extents we shared to the
> >>> +  * scratch file so they are returned to free space.
> >>> +  */
> >>> + ret = fallocate(scratch_fd,
> >>> + FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE, seg_off,
> >>> + seg_size);
> >> 
> >> Indentation here (two tabs for a continuation).  
> > 
> > OK.
> > 
> >> Or just ftruncate
> >> scratch_fd to zero bytes?  I think you have to do that for the EOF stuff
> >> to work, right?
> >> 
> > 
> > I’d truncate the UNSHARE range only in the loop.
> > EOF stuff would be truncated on (O_TMPFILE) file close.
> > The EOF stuff would be used for another purpose, see 
> > [PATCH 6/9] spaceman/defrag: workaround kernel
> > 
> > Thanks,
> > Wengang
> > 
> >> --D
> >> 
> >>> + if (ret != 0) {
> >>> + fprintf(stderr, "PUNCH_HOLE failed %s\n",
> >>> + strerror(errno));
> >>> + break;
> >>> + }
> >>> +
> >>> + /* for defrag stats */
> >>> + nr_seg_defrag += 1;
> >>> +
> >>> + /* for time stats */
> >>> + time_delta = get_time_delta_us(&t_punch_hole, &t_clone);
> >>> + if (time_delta > max_punch_us)
> >>> + max_punch_us = time_delta;
> >>> +
> >>> + if (stop)
> >>> + break;
> >>> } while (true);
> >>> out:
> >>> if (scratch_fd != -1) {
> >>> -- 
> >>> 2.39.3 (Apple Git-146)
> 
>