* [PATCH 4/4] fsx: move range generation logic into a common helper
@ 2020-04-08 10:36 fdmanana
2020-04-08 15:48 ` Darrick J. Wong
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: fdmanana @ 2020-04-08 10:36 UTC (permalink / raw)
To: fstests; +Cc: linux-btrfs, Filipe Manana
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) {
+ *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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-08 10:36 [PATCH 4/4] fsx: move range generation logic into a common helper fdmanana
@ 2020-04-08 15:48 ` Darrick J. Wong
2020-04-08 16:57 ` Filipe Manana
2020-04-08 18:12 ` [PATCH v2 " fdmanana
2020-04-17 17:32 ` [PATCH " fdmanana
2 siblings, 1 reply; 10+ messages in thread
From: Darrick J. Wong @ 2020-04-08 15:48 UTC (permalink / raw)
To: fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-08 15:48 ` Darrick J. Wong
@ 2020-04-08 16:57 ` Filipe Manana
2020-04-08 17:22 ` Darrick J. Wong
0 siblings, 1 reply; 10+ messages in thread
From: Filipe Manana @ 2020-04-08 16:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: fstests, linux-btrfs, Filipe Manana
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
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-08 16:57 ` Filipe Manana
@ 2020-04-08 17:22 ` Darrick J. Wong
0 siblings, 0 replies; 10+ messages in thread
From: Darrick J. Wong @ 2020-04-08 17:22 UTC (permalink / raw)
To: Filipe Manana; +Cc: fstests, linux-btrfs, Filipe Manana
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
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 4/4] fsx: move range generation logic into a common helper
2020-04-08 10:36 [PATCH 4/4] fsx: move range generation logic into a common helper fdmanana
2020-04-08 15:48 ` Darrick J. Wong
@ 2020-04-08 18:12 ` fdmanana
2020-04-17 17:10 ` Brian Foster
2020-04-17 17:32 ` [PATCH " fdmanana
2 siblings, 1 reply; 10+ messages in thread
From: fdmanana @ 2020-04-08 18:12 UTC (permalink / raw)
To: fstests; +Cc: linux-btrfs, Filipe Manana
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>
---
V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
ltp/fsx.c | 91 +++++++++++++++++++++++++--------------------------------------
1 file changed, 36 insertions(+), 55 deletions(-)
diff --git a/ltp/fsx.c b/ltp/fsx.c
index 89a5f60e..9bfc98e0 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(bool readbdy_align,
+ 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 (readbdy_align) {
+ *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(false, 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(false, 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(true, maxfilelen, &offset, &size, &offset2);
+ break;
}
have_op:
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] fsx: move range generation logic into a common helper
2020-04-08 18:12 ` [PATCH v2 " fdmanana
@ 2020-04-17 17:10 ` Brian Foster
2020-04-17 17:20 ` Filipe Manana
0 siblings, 1 reply; 10+ messages in thread
From: Brian Foster @ 2020-04-17 17:10 UTC (permalink / raw)
To: fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana
On Wed, Apr 08, 2020 at 07:12:08PM +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>
> ---
>
> V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
>
> ltp/fsx.c | 91 +++++++++++++++++++++++++--------------------------------------
> 1 file changed, 36 insertions(+), 55 deletions(-)
>
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 89a5f60e..9bfc98e0 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
...
> @@ -2004,63 +2034,14 @@ test(void)
> keep_size = random() % 2;
> 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;
It looks like this writebdy bit is lost in the new helper...
Brian
> - } while (range_overlaps(offset, offset2, size) ||
> - offset2 + size > maxfilelen);
> - break;
> - }
> + generate_dest_range(true, maxfilelen, &offset, &size, &offset2);
> + break;
> }
>
> have_op:
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 4/4] fsx: move range generation logic into a common helper
2020-04-17 17:10 ` Brian Foster
@ 2020-04-17 17:20 ` Filipe Manana
0 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2020-04-17 17:20 UTC (permalink / raw)
To: Brian Foster; +Cc: fstests, linux-btrfs, Filipe Manana
On Fri, Apr 17, 2020 at 6:10 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Wed, Apr 08, 2020 at 07:12:08PM +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>
> > ---
> >
> > V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
> >
> > ltp/fsx.c | 91 +++++++++++++++++++++++++--------------------------------------
> > 1 file changed, 36 insertions(+), 55 deletions(-)
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 89a5f60e..9bfc98e0 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> ...
> > @@ -2004,63 +2034,14 @@ test(void)
> > keep_size = random() % 2;
> > 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;
>
> It looks like this writebdy bit is lost in the new helper...
Yes, thanks for pointing out. I'll send a v3.
>
> Brian
>
> > - } while (range_overlaps(offset, offset2, size) ||
> > - offset2 + size > maxfilelen);
> > - break;
> > - }
> > + generate_dest_range(true, maxfilelen, &offset, &size, &offset2);
> > + break;
> > }
> >
> > have_op:
> > --
> > 2.11.0
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-08 10:36 [PATCH 4/4] fsx: move range generation logic into a common helper fdmanana
2020-04-08 15:48 ` Darrick J. Wong
2020-04-08 18:12 ` [PATCH v2 " fdmanana
@ 2020-04-17 17:32 ` fdmanana
2020-04-20 14:33 ` Brian Foster
2 siblings, 1 reply; 10+ messages in thread
From: fdmanana @ 2020-04-17 17:32 UTC (permalink / raw)
To: fstests; +Cc: linux-btrfs, Filipe Manana
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>
---
V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
V3: Added destination offset align by writebdy when bdy_align is true.
ltp/fsx.c | 94 ++++++++++++++++++++++++++-------------------------------------
1 file changed, 39 insertions(+), 55 deletions(-)
diff --git a/ltp/fsx.c b/ltp/fsx.c
index 89a5f60e..2e51169b 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1930,6 +1930,39 @@ range_overlaps(
return llabs((unsigned long long)off1 - off0) < size;
}
+static void generate_dest_range(bool bdy_align,
+ 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 (bdy_align) {
+ *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);
+ if (bdy_align)
+ *dst_offset = *dst_offset & writebdy;
+ else
+ *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 +2037,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(false, 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(false, 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(true, maxfilelen, &offset, &size, &offset2);
+ break;
}
have_op:
--
2.11.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-17 17:32 ` [PATCH " fdmanana
@ 2020-04-20 14:33 ` Brian Foster
2020-04-20 17:05 ` Filipe Manana
0 siblings, 1 reply; 10+ messages in thread
From: Brian Foster @ 2020-04-20 14:33 UTC (permalink / raw)
To: fdmanana; +Cc: fstests, linux-btrfs, Filipe Manana
On Fri, Apr 17, 2020 at 06:32:21PM +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>
> ---
>
> V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
> V3: Added destination offset align by writebdy when bdy_align is true.
>
> ltp/fsx.c | 94 ++++++++++++++++++++++++++-------------------------------------
> 1 file changed, 39 insertions(+), 55 deletions(-)
>
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 89a5f60e..2e51169b 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1930,6 +1930,39 @@ range_overlaps(
> return llabs((unsigned long long)off1 - off0) < size;
> }
>
> +static void generate_dest_range(bool bdy_align,
> + 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 (bdy_align) {
> + *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);
> + if (bdy_align)
> + *dst_offset = *dst_offset & writebdy;
That still doesn't look right (and either way we might as well use
consistent logic for readbdy and writebdy).
Brian
> + else
> + *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 +2037,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(false, 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(false, 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(true, maxfilelen, &offset, &size, &offset2);
> + break;
> }
>
> have_op:
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] fsx: move range generation logic into a common helper
2020-04-20 14:33 ` Brian Foster
@ 2020-04-20 17:05 ` Filipe Manana
0 siblings, 0 replies; 10+ messages in thread
From: Filipe Manana @ 2020-04-20 17:05 UTC (permalink / raw)
To: Brian Foster; +Cc: fstests, linux-btrfs, Filipe Manana
On Mon, Apr 20, 2020 at 3:34 PM Brian Foster <bfoster@redhat.com> wrote:
>
> On Fri, Apr 17, 2020 at 06:32:21PM +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>
> > ---
> >
> > V2: Turned the first parameter of the helper into a boolean as Darrick suggested.
> > V3: Added destination offset align by writebdy when bdy_align is true.
> >
> > ltp/fsx.c | 94 ++++++++++++++++++++++++++-------------------------------------
> > 1 file changed, 39 insertions(+), 55 deletions(-)
> >
> > diff --git a/ltp/fsx.c b/ltp/fsx.c
> > index 89a5f60e..2e51169b 100644
> > --- a/ltp/fsx.c
> > +++ b/ltp/fsx.c
> > @@ -1930,6 +1930,39 @@ range_overlaps(
> > return llabs((unsigned long long)off1 - off0) < size;
> > }
> >
> > +static void generate_dest_range(bool bdy_align,
> > + 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 (bdy_align) {
> > + *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);
> > + if (bdy_align)
> > + *dst_offset = *dst_offset & writebdy;
>
> That still doesn't look right (and either way we might as well use
> consistent logic for readbdy and writebdy).
Right, I made the mistakes of using & instead of % and = instead of
-=. They were not intentional, just a friday afternoon type of thing.
Thanks, correct in the next version.
>
> Brian
>
> > + else
> > + *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 +2037,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(false, 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(false, 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(true, maxfilelen, &offset, &size, &offset2);
> > + break;
> > }
> >
> > have_op:
> > --
> > 2.11.0
> >
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-04-20 17:05 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-08 10:36 [PATCH 4/4] fsx: move range generation logic into a common helper fdmanana
2020-04-08 15:48 ` Darrick J. Wong
2020-04-08 16:57 ` Filipe Manana
2020-04-08 17:22 ` Darrick J. Wong
2020-04-08 18:12 ` [PATCH v2 " fdmanana
2020-04-17 17:10 ` Brian Foster
2020-04-17 17:20 ` Filipe Manana
2020-04-17 17:32 ` [PATCH " fdmanana
2020-04-20 14:33 ` Brian Foster
2020-04-20 17:05 ` Filipe Manana
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.