All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mkfs: print newline if discard fails
@ 2019-12-14 18:05 Darrick J. Wong
  2019-12-14 18:34 ` Eric Sandeen
  0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-12-14 18:05 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure the "Discarding..." gets a newline after it no matter how we
exit the function.  Don't bother with any printing it even a small
discard request fails.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/xfs_mkfs.c |   12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4bfdebf6..948a5a77 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1251,6 +1251,14 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 	fd = libxfs_device_to_fd(dev);
 	if (fd <= 0)
 		return;
+
+	/*
+	 * Try discarding the first 64k; if that fails, don't bother printing
+	 * any messages at all.
+	 */
+	if (platform_discard_blocks(fd, offset, 65536))
+		return;
+
 	if (!quiet) {
 		printf("Discarding blocks...");
 		fflush(stdout);
@@ -1267,8 +1275,10 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 		 * not necessary for the mkfs functionality but just an
 		 * optimization. However we should stop on error.
 		 */
-		if (platform_discard_blocks(fd, offset, tmp_step))
+		if (platform_discard_blocks(fd, offset, tmp_step)) {
+			printf("\n");
 			return;
+		}
 
 		offset += tmp_step;
 	}

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] mkfs: print newline if discard fails
  2019-12-14 18:05 [PATCH] mkfs: print newline if discard fails Darrick J. Wong
@ 2019-12-14 18:34 ` Eric Sandeen
  2019-12-14 18:47   ` [PATCH] mkfs: tidy up discard notifications Eric Sandeen
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2019-12-14 18:34 UTC (permalink / raw)
  To: Darrick J. Wong, Eric Sandeen; +Cc: xfs

On 12/14/19 12:05 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make sure the "Discarding..." gets a newline after it no matter how we
> exit the function.  Don't bother with any printing it even a small
> discard request fails.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  mkfs/xfs_mkfs.c |   12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 4bfdebf6..948a5a77 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1251,6 +1251,14 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>  	fd = libxfs_device_to_fd(dev);
>  	if (fd <= 0)
>  		return;
> +
> +	/*
> +	 * Try discarding the first 64k; if that fails, don't bother printing
> +	 * any messages at all.
> +	 */
> +	if (platform_discard_blocks(fd, offset, 65536))
> +		return;

or trying to discard at all for that matter ;)  (i.e. this does more than suppress
messages)

I think this patch does 2 things... and that comment is a little confusing.

Also you discard the first 64k twice which is ok but...?

>  	if (!quiet) {
>  		printf("(ks...");
>  		fflush(stdout);
> @@ -1267,8 +1275,10 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>  		 * not necessary for the mkfs functionality but just an
>  		 * optimization. However we should stop on error.
>  		 */
> -		if (platform_discard_blocks(fd, offset, tmp_step))
> +		if (platform_discard_blocks(fd, offset, tmp_step)) {

also we don't want to do this if (quiet) ;)

> +			printf("\n");
>  			return;
> +		}
>  
>  		offset += tmp_step;
>  	}
> 

How about this, though I'd split it into 2 patches, 1 to catch the newline
if discard fails (and !quiet), and another to only print status if/after
the first discard succeeds


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4bfdebf6..9f07f042 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1244,6 +1244,7 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 {
 	int		fd;
 	uint64_t	offset = 0;
+	bool		messaged = false;
 	/* Discard the device 2G at a time */
 	const uint64_t	step = 2ULL << 30;
 	const uint64_t	count = BBTOB(nsectors);
@@ -1251,10 +1252,6 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 	fd = libxfs_device_to_fd(dev);
 	if (fd <= 0)
 		return;
-	if (!quiet) {
-		printf("Discarding blocks...");
-		fflush(stdout);
-	}
 
 	/* The block discarding happens in smaller batches so it can be
 	 * interrupted prematurely
@@ -1267,12 +1264,20 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 		 * not necessary for the mkfs functionality but just an
 		 * optimization. However we should stop on error.
 		 */
-		if (platform_discard_blocks(fd, offset, tmp_step))
+		if (platform_discard_blocks(fd, offset, tmp_step) == 0) {
+			if (!messaged && !quiet) {
+				printf("Discarding blocks...");
+				fflush(stdout);
+				messaged = true;
+			}
+		} else if (messaged && !quiet) {
+			printf("\n");
 			return;
+		}
 
 		offset += tmp_step;
 	}
-	if (!quiet)
+	if (messaged && !quiet)
 		printf("Done.\n");
 }
 


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH] mkfs: tidy up discard notifications
  2019-12-14 18:34 ` Eric Sandeen
@ 2019-12-14 18:47   ` Eric Sandeen
  2019-12-14 18:53     ` [PATCH V2] " Eric Sandeen
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2019-12-14 18:47 UTC (permalink / raw)
  To: Darrick J. Wong, Eric Sandeen; +Cc: xfs

Only notify user of discard operations if the first one succeeds,
and be sure to print a trailing newline if we stop early.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4bfdebf6..afa7feb4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1251,10 +1251,6 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 	fd = libxfs_device_to_fd(dev);
 	if (fd <= 0)
 		return;
-	if (!quiet) {
-		printf("Discarding blocks...");
-		fflush(stdout);
-	}
 
 	/* The block discarding happens in smaller batches so it can be
 	 * interrupted prematurely
@@ -1267,8 +1263,16 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 		 * not necessary for the mkfs functionality but just an
 		 * optimization. However we should stop on error.
 		 */
-		if (platform_discard_blocks(fd, offset, tmp_step))
+		if (platform_discard_blocks(fd, offset, tmp_step) == 0) {
+			if (offset == 0 && !quiet) {
+				printf("Discarding blocks...");
+				fflush(stdout);
+			}
+		} else if (!quiet) {
+			if (offset != 0)
+				printf("\n");
 			return;
+		}
 
 		offset += tmp_step;
 	}


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH V2] mkfs: tidy up discard notifications
  2019-12-14 18:47   ` [PATCH] mkfs: tidy up discard notifications Eric Sandeen
@ 2019-12-14 18:53     ` Eric Sandeen
  2019-12-14 20:21       ` Darrick J. Wong
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2019-12-14 18:53 UTC (permalink / raw)
  To: Darrick J. Wong, Eric Sandeen; +Cc: xfs

Only notify user of discard operations if the first one succeeds,
and be sure to print a trailing newline if we stop early.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V2: Logic is hard.  ;)  If I messed this one up, take it back Darrick.  :)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 4bfdebf6..606f79da 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -1251,10 +1251,6 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 	fd = libxfs_device_to_fd(dev);
 	if (fd <= 0)
 		return;
-	if (!quiet) {
-		printf("Discarding blocks...");
-		fflush(stdout);
-	}
 
 	/* The block discarding happens in smaller batches so it can be
 	 * interrupted prematurely
@@ -1267,12 +1263,20 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
 		 * not necessary for the mkfs functionality but just an
 		 * optimization. However we should stop on error.
 		 */
-		if (platform_discard_blocks(fd, offset, tmp_step))
+		if (platform_discard_blocks(fd, offset, tmp_step) == 0) {
+			if (offset == 0 && !quiet) {
+				printf("Discarding blocks...");
+				fflush(stdout);
+			}
+		} else {
+			if (offset > 0 && !quiet)
+				printf("\n");
 			return;
+		}
 
 		offset += tmp_step;
 	}
-	if (!quiet)
+	if (offset > 0 && !quiet)
 		printf("Done.\n");
 }
 



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH V2] mkfs: tidy up discard notifications
  2019-12-14 18:53     ` [PATCH V2] " Eric Sandeen
@ 2019-12-14 20:21       ` Darrick J. Wong
  2019-12-14 22:24         ` Eric Sandeen
  0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2019-12-14 20:21 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs

On Sat, Dec 14, 2019 at 12:53:56PM -0600, Eric Sandeen wrote:
> Only notify user of discard operations if the first one succeeds,
> and be sure to print a trailing newline if we stop early.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> V2: Logic is hard.  ;)  If I messed this one up, take it back Darrick.  :)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 4bfdebf6..606f79da 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -1251,10 +1251,6 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>  	fd = libxfs_device_to_fd(dev);
>  	if (fd <= 0)
>  		return;
> -	if (!quiet) {
> -		printf("Discarding blocks...");
> -		fflush(stdout);
> -	}
>  
>  	/* The block discarding happens in smaller batches so it can be
>  	 * interrupted prematurely
> @@ -1267,12 +1263,20 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>  		 * not necessary for the mkfs functionality but just an
>  		 * optimization. However we should stop on error.
>  		 */
> -		if (platform_discard_blocks(fd, offset, tmp_step))
> +		if (platform_discard_blocks(fd, offset, tmp_step) == 0) {
> +			if (offset == 0 && !quiet) {
> +				printf("Discarding blocks...");
> +				fflush(stdout);
> +			}
> +		} else {
> +			if (offset > 0 && !quiet)
> +				printf("\n");

Maybe we should say failed?  Or ... eh... whatever, the format happens
regardless.

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

>  			return;
> +		}
>  
>  		offset += tmp_step;
>  	}
> -	if (!quiet)
> +	if (offset > 0 && !quiet)
>  		printf("Done.\n");
>  }
>  
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH V2] mkfs: tidy up discard notifications
  2019-12-14 20:21       ` Darrick J. Wong
@ 2019-12-14 22:24         ` Eric Sandeen
  0 siblings, 0 replies; 6+ messages in thread
From: Eric Sandeen @ 2019-12-14 22:24 UTC (permalink / raw)
  To: Darrick J. Wong; +Cc: Eric Sandeen, xfs

On 12/14/19 2:21 PM, Darrick J. Wong wrote:
> On Sat, Dec 14, 2019 at 12:53:56PM -0600, Eric Sandeen wrote:
>> Only notify user of discard operations if the first one succeeds,
>> and be sure to print a trailing newline if we stop early.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> V2: Logic is hard.  ;)  If I messed this one up, take it back Darrick.  :)
>>
>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>> index 4bfdebf6..606f79da 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -1251,10 +1251,6 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>>  	fd = libxfs_device_to_fd(dev);
>>  	if (fd <= 0)
>>  		return;
>> -	if (!quiet) {
>> -		printf("Discarding blocks...");
>> -		fflush(stdout);
>> -	}
>>  
>>  	/* The block discarding happens in smaller batches so it can be
>>  	 * interrupted prematurely
>> @@ -1267,12 +1263,20 @@ discard_blocks(dev_t dev, uint64_t nsectors, int quiet)
>>  		 * not necessary for the mkfs functionality but just an
>>  		 * optimization. However we should stop on error.
>>  		 */
>> -		if (platform_discard_blocks(fd, offset, tmp_step))
>> +		if (platform_discard_blocks(fd, offset, tmp_step) == 0) {
>> +			if (offset == 0 && !quiet) {
>> +				printf("Discarding blocks...");
>> +				fflush(stdout);
>> +			}
>> +		} else {
>> +			if (offset > 0 && !quiet)
>> +				printf("\n");
> 
> Maybe we should say failed?  Or ... eh... whatever, the format happens
> regardless.

Yeah... a "failed" printf will launch support calls & emails.  :)

> Looks ok,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks Darrick.

-Eric

> --D
> 
>>  			return;
>> +		}
>>  
>>  		offset += tmp_step;
>>  	}
>> -	if (!quiet)
>> +	if (offset > 0 && !quiet)
>>  		printf("Done.\n");
>>  }
>>  
>>
>>
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-14 22:24 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-14 18:05 [PATCH] mkfs: print newline if discard fails Darrick J. Wong
2019-12-14 18:34 ` Eric Sandeen
2019-12-14 18:47   ` [PATCH] mkfs: tidy up discard notifications Eric Sandeen
2019-12-14 18:53     ` [PATCH V2] " Eric Sandeen
2019-12-14 20:21       ` Darrick J. Wong
2019-12-14 22:24         ` Eric Sandeen

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.