FSTests Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops
@ 2019-08-24 14:41 Eryu Guan
  2019-09-26  9:24 ` Eryu Guan
  2019-09-26 14:37 ` Brian Foster
  0 siblings, 2 replies; 3+ messages in thread
From: Eryu Guan @ 2019-08-24 14:41 UTC (permalink / raw)
  To: fstests; +Cc: Eryu Guan

From: Eryu Guan <eguan@linux.alibaba.com>

In CLONE/DEDUPE/COPY RANGE operations, we pick a "offset" and "size"
first, then find a suitable "offset2" by looping if there's overlap
(|offset2-offset| < size) or final file size is greater than max
file size (offset2 + size > maxfilelen).

But it's possible that there's no such suitable offset2 and we loop
forever. e.g. block_size = 4096, offset = 0, size = 4096 and
maxfilelen is a value smaller than 8212 (which could be set via '-l'
option).

Fix it by making sure maxfilelen/file_size is big enough to hold
'size' bytes from 'offset2', and just skip this operation if not.

Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
---
v2:
- don't use macro with ugly hacks, use an inline function instead

 ltp/fsx.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/ltp/fsx.c b/ltp/fsx.c
index 06d08e4e93f3..890666ab6140 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -1911,6 +1911,15 @@ fail:
 	return 0;
 }
 
+/* Check if range operations are possible to find a suitable offset */
+static inline bool check_range(unsigned long op, unsigned long off,
+			       unsigned long len, unsigned long size)
+{
+	bool ret = ((off + len * 2) <= size);
+	if (!ret)
+		log5(op, off, len, -1, FL_SKIPPED);
+	return ret;
+}
 
 int
 test(void)
@@ -1989,6 +1998,8 @@ test(void)
 		TRIM_OFF_LEN(offset, size, file_size);
 		offset = offset & ~(block_size - 1);
 		size = size & ~(block_size - 1);
+		if (!check_range(op, offset, size, maxfilelen))
+			goto out;
 		do {
 			offset2 = random();
 			TRIM_OFF(offset2, maxfilelen);
@@ -2003,6 +2014,8 @@ test(void)
 			TRIM_OFF_LEN(offset, size, file_size);
 			offset = offset & ~(block_size - 1);
 			size = size & ~(block_size - 1);
+			if (!check_range(op, offset, size, file_size))
+				goto out;
 			do {
 				if (tries++ >= 30) {
 					size = 0;
@@ -2020,6 +2033,8 @@ test(void)
 		offset -= offset % readbdy;
 		if (o_direct)
 			size -= size % readbdy;
+		if (!check_range(op, offset, size, maxfilelen))
+			goto out;
 		do {
 			offset2 = random();
 			TRIM_OFF(offset2, maxfilelen);
-- 
2.21.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops
  2019-08-24 14:41 [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops Eryu Guan
@ 2019-09-26  9:24 ` Eryu Guan
  2019-09-26 14:37 ` Brian Foster
  1 sibling, 0 replies; 3+ messages in thread
From: Eryu Guan @ 2019-09-26  9:24 UTC (permalink / raw)
  To: Eryu Guan; +Cc: fstests

On Sat, Aug 24, 2019 at 10:41:07PM +0800, Eryu Guan wrote:
> From: Eryu Guan <eguan@linux.alibaba.com>
> 
> In CLONE/DEDUPE/COPY RANGE operations, we pick a "offset" and "size"
> first, then find a suitable "offset2" by looping if there's overlap
> (|offset2-offset| < size) or final file size is greater than max
> file size (offset2 + size > maxfilelen).
> 
> But it's possible that there's no such suitable offset2 and we loop
> forever. e.g. block_size = 4096, offset = 0, size = 4096 and
> maxfilelen is a value smaller than 8212 (which could be set via '-l'
> option).
> 
> Fix it by making sure maxfilelen/file_size is big enough to hold
> 'size' bytes from 'offset2', and just skip this operation if not.
> 
> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>

Ping on this patch.

Eryu

> ---
> v2:
> - don't use macro with ugly hacks, use an inline function instead
> 
>  ltp/fsx.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 06d08e4e93f3..890666ab6140 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1911,6 +1911,15 @@ fail:
>  	return 0;
>  }
>  
> +/* Check if range operations are possible to find a suitable offset */
> +static inline bool check_range(unsigned long op, unsigned long off,
> +			       unsigned long len, unsigned long size)
> +{
> +	bool ret = ((off + len * 2) <= size);
> +	if (!ret)
> +		log5(op, off, len, -1, FL_SKIPPED);
> +	return ret;
> +}
>  
>  int
>  test(void)
> @@ -1989,6 +1998,8 @@ test(void)
>  		TRIM_OFF_LEN(offset, size, file_size);
>  		offset = offset & ~(block_size - 1);
>  		size = size & ~(block_size - 1);
> +		if (!check_range(op, offset, size, maxfilelen))
> +			goto out;
>  		do {
>  			offset2 = random();
>  			TRIM_OFF(offset2, maxfilelen);
> @@ -2003,6 +2014,8 @@ test(void)
>  			TRIM_OFF_LEN(offset, size, file_size);
>  			offset = offset & ~(block_size - 1);
>  			size = size & ~(block_size - 1);
> +			if (!check_range(op, offset, size, file_size))
> +				goto out;
>  			do {
>  				if (tries++ >= 30) {
>  					size = 0;
> @@ -2020,6 +2033,8 @@ test(void)
>  		offset -= offset % readbdy;
>  		if (o_direct)
>  			size -= size % readbdy;
> +		if (!check_range(op, offset, size, maxfilelen))
> +			goto out;
>  		do {
>  			offset2 = random();
>  			TRIM_OFF(offset2, maxfilelen);
> -- 
> 2.21.0

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops
  2019-08-24 14:41 [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops Eryu Guan
  2019-09-26  9:24 ` Eryu Guan
@ 2019-09-26 14:37 ` Brian Foster
  1 sibling, 0 replies; 3+ messages in thread
From: Brian Foster @ 2019-09-26 14:37 UTC (permalink / raw)
  To: Eryu Guan; +Cc: fstests, Eryu Guan

On Sat, Aug 24, 2019 at 10:41:07PM +0800, Eryu Guan wrote:
> From: Eryu Guan <eguan@linux.alibaba.com>
> 
> In CLONE/DEDUPE/COPY RANGE operations, we pick a "offset" and "size"
> first, then find a suitable "offset2" by looping if there's overlap
> (|offset2-offset| < size) or final file size is greater than max
> file size (offset2 + size > maxfilelen).
> 
> But it's possible that there's no such suitable offset2 and we loop
> forever. e.g. block_size = 4096, offset = 0, size = 4096 and
> maxfilelen is a value smaller than 8212 (which could be set via '-l'
> option).
> 
> Fix it by making sure maxfilelen/file_size is big enough to hold
> 'size' bytes from 'offset2', and just skip this operation if not.
> 
> Signed-off-by: Eryu Guan <eguan@linux.alibaba.com>
> ---
> v2:
> - don't use macro with ugly hacks, use an inline function instead
> 
>  ltp/fsx.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 06d08e4e93f3..890666ab6140 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -1911,6 +1911,15 @@ fail:
>  	return 0;
>  }
>  
> +/* Check if range operations are possible to find a suitable offset */
> +static inline bool check_range(unsigned long op, unsigned long off,
> +			       unsigned long len, unsigned long size)
> +{
> +	bool ret = ((off + len * 2) <= size);

A more self-documenting name than ret would be helpful, though I'd
probably just do something like:

	if ((off + len * 2) > size) {
		log5(op, off, len, -1, FL_SKIPPED);
		return false;
	}
	...
	return true;

> +	if (!ret)
> +		log5(op, off, len, -1, FL_SKIPPED);
> +	return ret;
> +}

Instead of offloading just this check, could we factor out the
associated offset2 loop into this function as well (and probably rename
it)? I.e., this function can return true if it assigned offset2 or false
and let the caller jump out. It could also take an optional max loop
count for the dedupe case.

Brian

>  
>  int
>  test(void)
> @@ -1989,6 +1998,8 @@ test(void)
>  		TRIM_OFF_LEN(offset, size, file_size);
>  		offset = offset & ~(block_size - 1);
>  		size = size & ~(block_size - 1);
> +		if (!check_range(op, offset, size, maxfilelen))
> +			goto out;
>  		do {
>  			offset2 = random();
>  			TRIM_OFF(offset2, maxfilelen);
> @@ -2003,6 +2014,8 @@ test(void)
>  			TRIM_OFF_LEN(offset, size, file_size);
>  			offset = offset & ~(block_size - 1);
>  			size = size & ~(block_size - 1);
> +			if (!check_range(op, offset, size, file_size))
> +				goto out;
>  			do {
>  				if (tries++ >= 30) {
>  					size = 0;
> @@ -2020,6 +2033,8 @@ test(void)
>  		offset -= offset % readbdy;
>  		if (o_direct)
>  			size -= size % readbdy;
> +		if (!check_range(op, offset, size, maxfilelen))
> +			goto out;
>  		do {
>  			offset2 = random();
>  			TRIM_OFF(offset2, maxfilelen);
> -- 
> 2.21.0
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-24 14:41 [PATCH v2] ltp/fsx: avoid infinite loop while finding offset2 in clone/dedupe/copy range ops Eryu Guan
2019-09-26  9:24 ` Eryu Guan
2019-09-26 14:37 ` Brian Foster

FSTests Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/fstests/0 fstests/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 fstests fstests/ https://lore.kernel.org/fstests \
		fstests@vger.kernel.org linux-fstests@archiver.kernel.org
	public-inbox-index fstests

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.fstests


AGPL code for this site: git clone https://public-inbox.org/ public-inbox