| Message ID | 20200408103627.11514-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] fsx: add missing file size update on zero range operations | expand |
On Wed, Apr 08, 2020 at 11:36:27AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > We have very similar code that generates the destination range for clone, > dedupe and copy_file_range operations, so avoid duplicating the code three > times and move it into a helper function. > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > ltp/fsx.c | 91 +++++++++++++++++++++++++-------------------------------------- > 1 file changed, 36 insertions(+), 55 deletions(-) > > diff --git a/ltp/fsx.c b/ltp/fsx.c > index 89a5f60e..8873cd01 100644 > --- a/ltp/fsx.c > +++ b/ltp/fsx.c > @@ -1930,6 +1930,36 @@ range_overlaps( > return llabs((unsigned long long)off1 - off0) < size; > } > > +static void generate_dest_range(unsigned long op, > + unsigned long max_range_end, > + unsigned long *src_offset, > + unsigned long *size, > + unsigned long *dst_offset) > +{ > + int tries = 0; > + > + TRIM_OFF_LEN(*src_offset, *size, file_size); > + if (op == OP_COPY_RANGE) { It feels funny that we pass in @op just to let COPY RANGE relax the block size alignment requirements. How about passing a block_align parameter (instead of op) which would be the subject of the if test? This way we concentrate the alignment requirement knowlege of each type of call in the "case OP_FOO" part of the code instead of scattering it? --D > + *src_offset -= *src_offset % readbdy; > + if (o_direct) > + *size -= *size % readbdy; > + } else { > + *src_offset = *src_offset & ~(block_size - 1); > + *size = *size & ~(block_size - 1); > + } > + > + do { > + if (tries++ >= 30) { > + *size = 0; > + break; > + } > + *dst_offset = random(); > + TRIM_OFF(*dst_offset, max_range_end); > + *dst_offset = *dst_offset & ~(block_size - 1); > + } while (range_overlaps(*src_offset, *dst_offset, *size) || > + *dst_offset + *size > max_range_end); > +} > + > int > test(void) > { > @@ -2004,63 +2034,14 @@ test(void) > keep_size = random() % 2; > break; > case OP_CLONE_RANGE: > - { > - int tries = 0; > - > - TRIM_OFF_LEN(offset, size, file_size); > - offset = offset & ~(block_size - 1); > - size = size & ~(block_size - 1); > - do { > - if (tries++ >= 30) { > - size = 0; > - break; > - } > - offset2 = random(); > - TRIM_OFF(offset2, maxfilelen); > - offset2 = offset2 & ~(block_size - 1); > - } while (range_overlaps(offset, offset2, size) || > - offset2 + size > maxfilelen); > - break; > - } > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > + break; > case OP_DEDUPE_RANGE: > - { > - int tries = 0; > - > - TRIM_OFF_LEN(offset, size, file_size); > - offset = offset & ~(block_size - 1); > - size = size & ~(block_size - 1); > - do { > - if (tries++ >= 30) { > - size = 0; > - break; > - } > - offset2 = random(); > - TRIM_OFF(offset2, file_size); > - offset2 = offset2 & ~(block_size - 1); > - } while (range_overlaps(offset, offset2, size) || > - offset2 + size > file_size); > - break; > - } > + generate_dest_range(op, file_size, &offset, &size, &offset2); > + break; > case OP_COPY_RANGE: > - { > - int tries = 0; > - > - TRIM_OFF_LEN(offset, size, file_size); > - offset -= offset % readbdy; > - if (o_direct) > - size -= size % readbdy; > - do { > - if (tries++ >= 30) { > - size = 0; > - break; > - } > - offset2 = random(); > - TRIM_OFF(offset2, maxfilelen); > - offset2 -= offset2 % writebdy; > - } while (range_overlaps(offset, offset2, size) || > - offset2 + size > maxfilelen); > - break; > - } > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > + break; > } > > have_op: > -- > 2.11.0 >
On Wed, Apr 8, 2020 at 4:48 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Wed, Apr 08, 2020 at 11:36:27AM +0100, fdmanana@kernel.org wrote: > > From: Filipe Manana <fdmanana@suse.com> > > > > We have very similar code that generates the destination range for clone, > > dedupe and copy_file_range operations, so avoid duplicating the code three > > times and move it into a helper function. > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > --- > > ltp/fsx.c | 91 +++++++++++++++++++++++++-------------------------------------- > > 1 file changed, 36 insertions(+), 55 deletions(-) > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > index 89a5f60e..8873cd01 100644 > > --- a/ltp/fsx.c > > +++ b/ltp/fsx.c > > @@ -1930,6 +1930,36 @@ range_overlaps( > > return llabs((unsigned long long)off1 - off0) < size; > > } > > > > +static void generate_dest_range(unsigned long op, > > + unsigned long max_range_end, > > + unsigned long *src_offset, > > + unsigned long *size, > > + unsigned long *dst_offset) > > +{ > > + int tries = 0; > > + > > + TRIM_OFF_LEN(*src_offset, *size, file_size); > > + if (op == OP_COPY_RANGE) { > > It feels funny that we pass in @op just to let COPY RANGE relax the > block size alignment requirements. How about passing a block_align > parameter (instead of op) which would be the subject of the if test? > This way we concentrate the alignment requirement knowlege of each type > of call in the "case OP_FOO" part of the code instead of scattering it? You mean something like the following: https://pastebin.com/e2NZKbZV ? > > --D > > > + *src_offset -= *src_offset % readbdy; > > + if (o_direct) > > + *size -= *size % readbdy; > > + } else { > > + *src_offset = *src_offset & ~(block_size - 1); > > + *size = *size & ~(block_size - 1); > > + } > > + > > + do { > > + if (tries++ >= 30) { > > + *size = 0; > > + break; > > + } > > + *dst_offset = random(); > > + TRIM_OFF(*dst_offset, max_range_end); > > + *dst_offset = *dst_offset & ~(block_size - 1); > > + } while (range_overlaps(*src_offset, *dst_offset, *size) || > > + *dst_offset + *size > max_range_end); > > +} > > + > > int > > test(void) > > { > > @@ -2004,63 +2034,14 @@ test(void) > > keep_size = random() % 2; > > break; > > case OP_CLONE_RANGE: > > - { > > - int tries = 0; > > - > > - TRIM_OFF_LEN(offset, size, file_size); > > - offset = offset & ~(block_size - 1); > > - size = size & ~(block_size - 1); > > - do { > > - if (tries++ >= 30) { > > - size = 0; > > - break; > > - } > > - offset2 = random(); > > - TRIM_OFF(offset2, maxfilelen); > > - offset2 = offset2 & ~(block_size - 1); > > - } while (range_overlaps(offset, offset2, size) || > > - offset2 + size > maxfilelen); > > - break; > > - } > > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > > + break; > > case OP_DEDUPE_RANGE: > > - { > > - int tries = 0; > > - > > - TRIM_OFF_LEN(offset, size, file_size); > > - offset = offset & ~(block_size - 1); > > - size = size & ~(block_size - 1); > > - do { > > - if (tries++ >= 30) { > > - size = 0; > > - break; > > - } > > - offset2 = random(); > > - TRIM_OFF(offset2, file_size); > > - offset2 = offset2 & ~(block_size - 1); > > - } while (range_overlaps(offset, offset2, size) || > > - offset2 + size > file_size); > > - break; > > - } > > + generate_dest_range(op, file_size, &offset, &size, &offset2); > > + break; > > case OP_COPY_RANGE: > > - { > > - int tries = 0; > > - > > - TRIM_OFF_LEN(offset, size, file_size); > > - offset -= offset % readbdy; > > - if (o_direct) > > - size -= size % readbdy; > > - do { > > - if (tries++ >= 30) { > > - size = 0; > > - break; > > - } > > - offset2 = random(); > > - TRIM_OFF(offset2, maxfilelen); > > - offset2 -= offset2 % writebdy; > > - } while (range_overlaps(offset, offset2, size) || > > - offset2 + size > maxfilelen); > > - break; > > - } > > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > > + break; > > } > > > > have_op: > > -- > > 2.11.0 > >
On Wed, Apr 08, 2020 at 05:57:26PM +0100, Filipe Manana wrote: > On Wed, Apr 8, 2020 at 4:48 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Wed, Apr 08, 2020 at 11:36:27AM +0100, fdmanana@kernel.org wrote: > > > From: Filipe Manana <fdmanana@suse.com> > > > > > > We have very similar code that generates the destination range for clone, > > > dedupe and copy_file_range operations, so avoid duplicating the code three > > > times and move it into a helper function. > > > > > > Signed-off-by: Filipe Manana <fdmanana@suse.com> > > > --- > > > ltp/fsx.c | 91 +++++++++++++++++++++++++-------------------------------------- > > > 1 file changed, 36 insertions(+), 55 deletions(-) > > > > > > diff --git a/ltp/fsx.c b/ltp/fsx.c > > > index 89a5f60e..8873cd01 100644 > > > --- a/ltp/fsx.c > > > +++ b/ltp/fsx.c > > > @@ -1930,6 +1930,36 @@ range_overlaps( > > > return llabs((unsigned long long)off1 - off0) < size; > > > } > > > > > > +static void generate_dest_range(unsigned long op, > > > + unsigned long max_range_end, > > > + unsigned long *src_offset, > > > + unsigned long *size, > > > + unsigned long *dst_offset) > > > +{ > > > + int tries = 0; > > > + > > > + TRIM_OFF_LEN(*src_offset, *size, file_size); > > > + if (op == OP_COPY_RANGE) { > > > > It feels funny that we pass in @op just to let COPY RANGE relax the > > block size alignment requirements. How about passing a block_align > > parameter (instead of op) which would be the subject of the if test? > > This way we concentrate the alignment requirement knowlege of each type > > of call in the "case OP_FOO" part of the code instead of scattering it? > > You mean something like the following: > > https://pastebin.com/e2NZKbZV > > ? Yep. --D > > > > > --D > > > > > + *src_offset -= *src_offset % readbdy; > > > + if (o_direct) > > > + *size -= *size % readbdy; > > > + } else { > > > + *src_offset = *src_offset & ~(block_size - 1); > > > + *size = *size & ~(block_size - 1); > > > + } > > > + > > > + do { > > > + if (tries++ >= 30) { > > > + *size = 0; > > > + break; > > > + } > > > + *dst_offset = random(); > > > + TRIM_OFF(*dst_offset, max_range_end); > > > + *dst_offset = *dst_offset & ~(block_size - 1); > > > + } while (range_overlaps(*src_offset, *dst_offset, *size) || > > > + *dst_offset + *size > max_range_end); > > > +} > > > + > > > int > > > test(void) > > > { > > > @@ -2004,63 +2034,14 @@ test(void) > > > keep_size = random() % 2; > > > break; > > > case OP_CLONE_RANGE: > > > - { > > > - int tries = 0; > > > - > > > - TRIM_OFF_LEN(offset, size, file_size); > > > - offset = offset & ~(block_size - 1); > > > - size = size & ~(block_size - 1); > > > - do { > > > - if (tries++ >= 30) { > > > - size = 0; > > > - break; > > > - } > > > - offset2 = random(); > > > - TRIM_OFF(offset2, maxfilelen); > > > - offset2 = offset2 & ~(block_size - 1); > > > - } while (range_overlaps(offset, offset2, size) || > > > - offset2 + size > maxfilelen); > > > - break; > > > - } > > > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > > > + break; > > > case OP_DEDUPE_RANGE: > > > - { > > > - int tries = 0; > > > - > > > - TRIM_OFF_LEN(offset, size, file_size); > > > - offset = offset & ~(block_size - 1); > > > - size = size & ~(block_size - 1); > > > - do { > > > - if (tries++ >= 30) { > > > - size = 0; > > > - break; > > > - } > > > - offset2 = random(); > > > - TRIM_OFF(offset2, file_size); > > > - offset2 = offset2 & ~(block_size - 1); > > > - } while (range_overlaps(offset, offset2, size) || > > > - offset2 + size > file_size); > > > - break; > > > - } > > > + generate_dest_range(op, file_size, &offset, &size, &offset2); > > > + break; > > > case OP_COPY_RANGE: > > > - { > > > - int tries = 0; > > > - > > > - TRIM_OFF_LEN(offset, size, file_size); > > > - offset -= offset % readbdy; > > > - if (o_direct) > > > - size -= size % readbdy; > > > - do { > > > - if (tries++ >= 30) { > > > - size = 0; > > > - break; > > > - } > > > - offset2 = random(); > > > - TRIM_OFF(offset2, maxfilelen); > > > - offset2 -= offset2 % writebdy; > > > - } while (range_overlaps(offset, offset2, size) || > > > - offset2 + size > maxfilelen); > > > - break; > > > - } > > > + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); > > > + break; > > > } > > > > > > have_op: > > > -- > > > 2.11.0 > > >
diff --git a/ltp/fsx.c b/ltp/fsx.c index 89a5f60e..8873cd01 100644 --- a/ltp/fsx.c +++ b/ltp/fsx.c @@ -1930,6 +1930,36 @@ range_overlaps( return llabs((unsigned long long)off1 - off0) < size; } +static void generate_dest_range(unsigned long op, + unsigned long max_range_end, + unsigned long *src_offset, + unsigned long *size, + unsigned long *dst_offset) +{ + int tries = 0; + + TRIM_OFF_LEN(*src_offset, *size, file_size); + if (op == OP_COPY_RANGE) { + *src_offset -= *src_offset % readbdy; + if (o_direct) + *size -= *size % readbdy; + } else { + *src_offset = *src_offset & ~(block_size - 1); + *size = *size & ~(block_size - 1); + } + + do { + if (tries++ >= 30) { + *size = 0; + break; + } + *dst_offset = random(); + TRIM_OFF(*dst_offset, max_range_end); + *dst_offset = *dst_offset & ~(block_size - 1); + } while (range_overlaps(*src_offset, *dst_offset, *size) || + *dst_offset + *size > max_range_end); +} + int test(void) { @@ -2004,63 +2034,14 @@ test(void) keep_size = random() % 2; break; case OP_CLONE_RANGE: - { - int tries = 0; - - TRIM_OFF_LEN(offset, size, file_size); - offset = offset & ~(block_size - 1); - size = size & ~(block_size - 1); - do { - if (tries++ >= 30) { - size = 0; - break; - } - offset2 = random(); - TRIM_OFF(offset2, maxfilelen); - offset2 = offset2 & ~(block_size - 1); - } while (range_overlaps(offset, offset2, size) || - offset2 + size > maxfilelen); - break; - } + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); + break; case OP_DEDUPE_RANGE: - { - int tries = 0; - - TRIM_OFF_LEN(offset, size, file_size); - offset = offset & ~(block_size - 1); - size = size & ~(block_size - 1); - do { - if (tries++ >= 30) { - size = 0; - break; - } - offset2 = random(); - TRIM_OFF(offset2, file_size); - offset2 = offset2 & ~(block_size - 1); - } while (range_overlaps(offset, offset2, size) || - offset2 + size > file_size); - break; - } + generate_dest_range(op, file_size, &offset, &size, &offset2); + break; case OP_COPY_RANGE: - { - int tries = 0; - - TRIM_OFF_LEN(offset, size, file_size); - offset -= offset % readbdy; - if (o_direct) - size -= size % readbdy; - do { - if (tries++ >= 30) { - size = 0; - break; - } - offset2 = random(); - TRIM_OFF(offset2, maxfilelen); - offset2 -= offset2 % writebdy; - } while (range_overlaps(offset, offset2, size) || - offset2 + size > maxfilelen); - break; - } + generate_dest_range(op, maxfilelen, &offset, &size, &offset2); + break; } have_op: