[PATCH 0/7] e2fsprogs: Using discard in e2fsprogs tools

[PATCH 0/7] e2fsprogs: Using discard in e2fsprogs tools

[Lukas Czerner]

Hi all,

We have came to consensus about using discard in e2fsprogs tools. These
patches generalize use of discard in e2fsprogs tools introduce changes
in mke2fs and e2fsck.

Short summary
-------------

 [PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager
   * generalize use of discard in e2fsprogs and let any tool in e2fsprogs
     take advantage of it withou need to write its own BLKDISCARD wrappers.

 [PATCH 2/7] e2fsprogs: Add discard_zeroes_data into struct_io_manager
   * Give the opportunity for any io_manager to check if device discard
     support zeroes data and save the results into io_manager itself for
     use in any e2fsprofs tool.

 [PATCH 3/7] e2fsck: Keep track of problems during the check

 [PATCH 4/7] e2fsck: Discard free data and inode blocks.
   * In pass 5 after the group descriptors has been changed discard free
     data and inode blocks. The consensus was that it should be OFF by
     default, so it is.
   * Introduce new paid of extended options discard/nodiscard.

 [PATCH 5/7] mke2fs: Change -K option to discard/nodiscard
   * To the same of consistency and to gain ability to easily default it
     the new pair of extended options has been added.
   * The consensus was that it should stay ON by default, so it is.

 [PATCH 6/7] mke2fs: Use unix_discard() for discards

 [PATCH 7/7] mke2fs: Use io_manager discard_zeroes_data property


Any comments appreciated!

Thanks!

-Lukas

---
[PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager
[PATCH 2/7] e2fsprogs: Add discard_zeroes_data into struct_io_manager
[PATCH 3/7] e2fsck: Keep track of problems during the check
[PATCH 4/7] e2fsck: Discard free data and inode blocks.
[PATCH 5/7] mke2fs: Change -K option to discard/nodiscard
[PATCH 6/7] mke2fs: Use unix_discard() for discards
[PATCH 7/7] mke2fs: Use io_manager discard_zeroes_data property

 e2fsck/e2fsck.8.in   |   14 ++++++++
 e2fsck/e2fsck.h      |    2 +
 e2fsck/pass5.c       |   81 +++++++++++++++++++++++++++++++++++++++++++++
 e2fsck/problem.c     |   31 ++++++++++++-----
 e2fsck/problem.h     |    3 ++
 e2fsck/problemP.h    |    1 +
 e2fsck/unix.c        |   10 +++++-
 lib/ext2fs/ext2_io.h |    3 ++
 lib/ext2fs/test_io.c |    1 +
 lib/ext2fs/unix_io.c |   53 +++++++++++++++++++++++++++++
 misc/mke2fs.8.in     |   18 ++++++----
 misc/mke2fs.c        |   90 +++++++++++--------------------------------------
 12 files changed, 219 insertions(+), 88 deletions(-)

[PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager

[Lukas Czerner]

In order to provide generic "discard" function for all e2fsprogs tools
add a discard function prototype into struct_io_manager. Specific
function for specific io managers can be crated that way.

This commit also creates unix_discard function which uses BLKDISCARD
ioctl to discard data blocks on the block device and bind it into
unit_io_manager structure to be available for all e2fsprogs tools.
Note that BLKDISCARD is still Linux specific ioctl, however other
unix systems may provide similar functionality. So far the
unix_discard() remains linux specific hence is embedded in #ifdef
__linux__ macro.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/ext2_io.h |    2 ++
 lib/ext2fs/unix_io.c |   41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
index ccc9c8b..d202007 100644
--- a/lib/ext2fs/ext2_io.h
+++ b/lib/ext2fs/ext2_io.h
@@ -83,6 +83,8 @@ struct struct_io_manager {
 					int count, void *data);
 	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
 					int count, const void *data);
+	errcode_t (*discard)(io_channel channel, unsigned long long block,
+			     unsigned long long count, const void *data);
 	long	reserved[16];
 };
 
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index 1df1fdd..5b6cdec 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
 			       int count, void *data);
 static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
 				int count, const void *data);
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			      unsigned long long count, const void *data);
 
 static struct struct_io_manager struct_unix_manager = {
 	EXT2_ET_MAGIC_IO_MANAGER,
@@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
 	unix_get_stats,
 	unix_read_blk64,
 	unix_write_blk64,
+	unix_discard,
 };
 
 io_manager unix_io_manager = &struct_unix_manager;
@@ -834,3 +837,41 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
 	}
 	return EXT2_ET_INVALID_ARGUMENT;
 }
+
+#ifdef __linux__
+
+#ifndef BLKDISCARD
+#define BLKDISCARD	_IO(0x12,119)
+#endif
+
+static errcode_t unix_discard(io_channel channel, unsigned long long block,
+			     unsigned long long count, const void *data)
+{
+
+	struct struct_ext2_filsys *fs;
+	struct unix_private_data *u_priv;
+	errcode_t	retval = 0;
+	__uint64_t	range[2];
+	int		blocksize;
+
+	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
+	u_priv = (struct unix_private_data *) channel->private_data;
+	EXT2_CHECK_MAGIC(u_priv, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
+
+	fs = (struct struct_ext2_filsys *) data;
+	if (!fs)
+		return EXT2_ET_INVALID_ARGUMENT;
+
+	blocksize = EXT2_BLOCK_SIZE(fs->super);
+
+	range[0] = (__uint64_t)(block);
+	range[1] = (__uint64_t)(count);
+	range[0] *= (__uint64_t)(blocksize);
+	range[1] *= (__uint64_t)(blocksize);
+
+	return ioctl(u_priv->dev, BLKDISCARD, &range);
+}
+
+#else
+#define unix_discard(channel, block, count)	EXT2_ET_UNIMPLEMENTED
+#endif
-- 
1.7.2.3

Re: [PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> In order to provide generic "discard" function for all e2fsprogs tools
> add a discard function prototype into struct_io_manager. Specific
> function for specific io managers can be crated that way.
> 
> This commit also creates unix_discard function which uses BLKDISCARD
> ioctl to discard data blocks on the block device and bind it into
> unit_io_manager structure to be available for all e2fsprogs tools.
> Note that BLKDISCARD is still Linux specific ioctl, however other
> unix systems may provide similar functionality. So far the
> unix_discard() remains linux specific hence is embedded in #ifdef
> __linux__ macro.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  lib/ext2fs/ext2_io.h |    2 ++
>  lib/ext2fs/unix_io.c |   41 +++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
> index ccc9c8b..d202007 100644
> --- a/lib/ext2fs/ext2_io.h
> +++ b/lib/ext2fs/ext2_io.h
> @@ -83,6 +83,8 @@ struct struct_io_manager {
>  					int count, void *data);
>  	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
>  					int count, const void *data);
> +	errcode_t (*discard)(io_channel channel, unsigned long long block,
> +			     unsigned long long count, const void *data);
>  	long	reserved[16];
>  };
>  
> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> index 1df1fdd..5b6cdec 100644
> --- a/lib/ext2fs/unix_io.c
> +++ b/lib/ext2fs/unix_io.c
> @@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
>  			       int count, void *data);
>  static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
>  				int count, const void *data);
> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> +			      unsigned long long count, const void *data);
>  
>  static struct struct_io_manager struct_unix_manager = {
>  	EXT2_ET_MAGIC_IO_MANAGER,
> @@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
>  	unix_get_stats,
>  	unix_read_blk64,
>  	unix_write_blk64,
> +	unix_discard,
>  };
>  
>  io_manager unix_io_manager = &struct_unix_manager;
> @@ -834,3 +837,41 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
>  	}
>  	return EXT2_ET_INVALID_ARGUMENT;
>  }
> +
> +#ifdef __linux__
> +
> +#ifndef BLKDISCARD
> +#define BLKDISCARD	_IO(0x12,119)
> +#endif
> +
> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> +			     unsigned long long count, const void *data)
> +{
> +
> +	struct struct_ext2_filsys *fs;
> +	struct unix_private_data *u_priv;
> +	errcode_t	retval = 0;
> +	__uint64_t	range[2];
> +	int		blocksize;
> +
> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> +	u_priv = (struct unix_private_data *) channel->private_data;
> +	EXT2_CHECK_MAGIC(u_priv, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
> +
> +	fs = (struct struct_ext2_filsys *) data;
> +	if (!fs)
> +		return EXT2_ET_INVALID_ARGUMENT;
> +
> +	blocksize = EXT2_BLOCK_SIZE(fs->super);

This seems a little convoluted; you pass in *data, which gets you the fs,
from which you get the super, from which you get the blocksize,
which is all that you ever actually use here:

> +	range[0] = (__uint64_t)(block);
> +	range[1] = (__uint64_t)(count);
> +	range[0] *= (__uint64_t)(blocksize);
> +	range[1] *= (__uint64_t)(blocksize);

any reason to not just pass in the blocksize?

Maybe this is for flexibility for not-linux, but I guess we don't know what
they need anyway...?

And if you do that you can change "u_priv" to "data" just to match the
other handlers, perhaps.

-Eric

> +	return ioctl(u_priv->dev, BLKDISCARD, &range);
> +}
> +
> +#else
> +#define unix_discard(channel, block, count)	EXT2_ET_UNIMPLEMENTED
> +#endif

Re: [PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager

[Lukas Czerner]

On Tue, 16 Nov 2010, Eric Sandeen wrote:

> On 10/26/10 12:54 PM, Lukas Czerner wrote:
> > In order to provide generic "discard" function for all e2fsprogs tools
> > add a discard function prototype into struct_io_manager. Specific
> > function for specific io managers can be crated that way.
> > 
> > This commit also creates unix_discard function which uses BLKDISCARD
> > ioctl to discard data blocks on the block device and bind it into
> > unit_io_manager structure to be available for all e2fsprogs tools.
> > Note that BLKDISCARD is still Linux specific ioctl, however other
> > unix systems may provide similar functionality. So far the
> > unix_discard() remains linux specific hence is embedded in #ifdef
> > __linux__ macro.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  lib/ext2fs/ext2_io.h |    2 ++
> >  lib/ext2fs/unix_io.c |   41 +++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+), 0 deletions(-)
> > 
> > diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
> > index ccc9c8b..d202007 100644
> > --- a/lib/ext2fs/ext2_io.h
> > +++ b/lib/ext2fs/ext2_io.h
> > @@ -83,6 +83,8 @@ struct struct_io_manager {
> >  					int count, void *data);
> >  	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
> >  					int count, const void *data);
> > +	errcode_t (*discard)(io_channel channel, unsigned long long block,
> > +			     unsigned long long count, const void *data);
> >  	long	reserved[16];
> >  };
> >  
> > diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> > index 1df1fdd..5b6cdec 100644
> > --- a/lib/ext2fs/unix_io.c
> > +++ b/lib/ext2fs/unix_io.c
> > @@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
> >  			       int count, void *data);
> >  static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
> >  				int count, const void *data);
> > +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> > +			      unsigned long long count, const void *data);
> >  
> >  static struct struct_io_manager struct_unix_manager = {
> >  	EXT2_ET_MAGIC_IO_MANAGER,
> > @@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
> >  	unix_get_stats,
> >  	unix_read_blk64,
> >  	unix_write_blk64,
> > +	unix_discard,
> >  };
> >  
> >  io_manager unix_io_manager = &struct_unix_manager;
> > @@ -834,3 +837,41 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
> >  	}
> >  	return EXT2_ET_INVALID_ARGUMENT;
> >  }
> > +
> > +#ifdef __linux__
> > +
> > +#ifndef BLKDISCARD
> > +#define BLKDISCARD	_IO(0x12,119)
> > +#endif
> > +
> > +static errcode_t unix_discard(io_channel channel, unsigned long long block,
> > +			     unsigned long long count, const void *data)
> > +{
> > +
> > +	struct struct_ext2_filsys *fs;
> > +	struct unix_private_data *u_priv;
> > +	errcode_t	retval = 0;
> > +	__uint64_t	range[2];
> > +	int		blocksize;
> > +
> > +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> > +	u_priv = (struct unix_private_data *) channel->private_data;
> > +	EXT2_CHECK_MAGIC(u_priv, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
> > +
> > +	fs = (struct struct_ext2_filsys *) data;
> > +	if (!fs)
> > +		return EXT2_ET_INVALID_ARGUMENT;
> > +
> > +	blocksize = EXT2_BLOCK_SIZE(fs->super);
> 
> This seems a little convoluted; you pass in *data, which gets you the fs,
> from which you get the super, from which you get the blocksize,
> which is all that you ever actually use here:
> 
> > +	range[0] = (__uint64_t)(block);
> > +	range[1] = (__uint64_t)(count);
> > +	range[0] *= (__uint64_t)(blocksize);
> > +	range[1] *= (__uint64_t)(blocksize);
> 
> any reason to not just pass in the blocksize?
> 
> Maybe this is for flexibility for not-linux, but I guess we don't know what
> they need anyway...?
> 
> And if you do that you can change "u_priv" to "data" just to match the
> other handlers, perhaps.

You're right, I was so closely following the example that I forget that
it can be done more simply. Does this looks good to you ?

static errcode_t unix_discard(io_channel channel, unsigned long long block,
			     unsigned long long count, const void *blocksize)
{
	unsigned int    *bsize = (unsigned int *) blocksize;
	struct unix_private_data *data;
	errcode_t	retval = 0;
	__uint64_t	range[2];

	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
	data = (struct unix_private_data *) channel->private_data;
	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);

	if (!blocksize)
		return EXT2_ET_INVALID_ARGUMENT;

	range[0] = (__uint64_t)(block);
	range[1] = (__uint64_t)(count);
	range[0] *= (__uint64_t)(*bsize);
	range[1] *= (__uint64_t)(*bsize);

	return ioctl(data->dev, BLKDISCARD, &range);
}

then we can call it like this:

ret = manager->discard(fs->io, start, count, &fs->blocksize);

Thanks a lot for reviewing this!

-Lukas

> 
> -Eric
> 
> > +	return ioctl(u_priv->dev, BLKDISCARD, &range);
> > +}
> > +
> > +#else
> > +#define unix_discard(channel, block, count)	EXT2_ET_UNIMPLEMENTED
> > +#endif
> 
> 

-- 

Re: [PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager

[Eric Sandeen]

On 11/16/10 2:16 PM, Lukas Czerner wrote:
> On Tue, 16 Nov 2010, Eric Sandeen wrote:
> 
>> On 10/26/10 12:54 PM, Lukas Czerner wrote:
>>> In order to provide generic "discard" function for all e2fsprogs tools
>>> add a discard function prototype into struct_io_manager. Specific
>>> function for specific io managers can be crated that way.
>>>
>>> This commit also creates unix_discard function which uses BLKDISCARD
>>> ioctl to discard data blocks on the block device and bind it into
>>> unit_io_manager structure to be available for all e2fsprogs tools.
>>> Note that BLKDISCARD is still Linux specific ioctl, however other
>>> unix systems may provide similar functionality. So far the
>>> unix_discard() remains linux specific hence is embedded in #ifdef
>>> __linux__ macro.
>>>
>>> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
>>> ---
>>>  lib/ext2fs/ext2_io.h |    2 ++
>>>  lib/ext2fs/unix_io.c |   41 +++++++++++++++++++++++++++++++++++++++++
>>>  2 files changed, 43 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
>>> index ccc9c8b..d202007 100644
>>> --- a/lib/ext2fs/ext2_io.h
>>> +++ b/lib/ext2fs/ext2_io.h
>>> @@ -83,6 +83,8 @@ struct struct_io_manager {
>>>  					int count, void *data);
>>>  	errcode_t (*write_blk64)(io_channel channel, unsigned long long block,
>>>  					int count, const void *data);
>>> +	errcode_t (*discard)(io_channel channel, unsigned long long block,
>>> +			     unsigned long long count, const void *data);
>>>  	long	reserved[16];
>>>  };
>>>  
>>> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
>>> index 1df1fdd..5b6cdec 100644
>>> --- a/lib/ext2fs/unix_io.c
>>> +++ b/lib/ext2fs/unix_io.c
>>> @@ -115,6 +115,8 @@ static errcode_t unix_read_blk64(io_channel channel, unsigned long long block,
>>>  			       int count, void *data);
>>>  static errcode_t unix_write_blk64(io_channel channel, unsigned long long block,
>>>  				int count, const void *data);
>>> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
>>> +			      unsigned long long count, const void *data);
>>>  
>>>  static struct struct_io_manager struct_unix_manager = {
>>>  	EXT2_ET_MAGIC_IO_MANAGER,
>>> @@ -130,6 +132,7 @@ static struct struct_io_manager struct_unix_manager = {
>>>  	unix_get_stats,
>>>  	unix_read_blk64,
>>>  	unix_write_blk64,
>>> +	unix_discard,
>>>  };
>>>  
>>>  io_manager unix_io_manager = &struct_unix_manager;
>>> @@ -834,3 +837,41 @@ static errcode_t unix_set_option(io_channel channel, const char *option,
>>>  	}
>>>  	return EXT2_ET_INVALID_ARGUMENT;
>>>  }
>>> +
>>> +#ifdef __linux__
>>> +
>>> +#ifndef BLKDISCARD
>>> +#define BLKDISCARD	_IO(0x12,119)
>>> +#endif
>>> +
>>> +static errcode_t unix_discard(io_channel channel, unsigned long long block,
>>> +			     unsigned long long count, const void *data)
>>> +{
>>> +
>>> +	struct struct_ext2_filsys *fs;
>>> +	struct unix_private_data *u_priv;
>>> +	errcode_t	retval = 0;
>>> +	__uint64_t	range[2];
>>> +	int		blocksize;
>>> +
>>> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
>>> +	u_priv = (struct unix_private_data *) channel->private_data;
>>> +	EXT2_CHECK_MAGIC(u_priv, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
>>> +
>>> +	fs = (struct struct_ext2_filsys *) data;
>>> +	if (!fs)
>>> +		return EXT2_ET_INVALID_ARGUMENT;
>>> +
>>> +	blocksize = EXT2_BLOCK_SIZE(fs->super);
>>
>> This seems a little convoluted; you pass in *data, which gets you the fs,
>> from which you get the super, from which you get the blocksize,
>> which is all that you ever actually use here:
>>
>>> +	range[0] = (__uint64_t)(block);
>>> +	range[1] = (__uint64_t)(count);
>>> +	range[0] *= (__uint64_t)(blocksize);
>>> +	range[1] *= (__uint64_t)(blocksize);
>>
>> any reason to not just pass in the blocksize?
>>
>> Maybe this is for flexibility for not-linux, but I guess we don't know what
>> they need anyway...?
>>
>> And if you do that you can change "u_priv" to "data" just to match the
>> other handlers, perhaps.
> 
> You're right, I was so closely following the example that I forget that
> it can be done more simply. Does this looks good to you ?
> 
> static errcode_t unix_discard(io_channel channel, unsigned long long block,
> 			     unsigned long long count, const void *blocksize)
> {
> 	unsigned int    *bsize = (unsigned int *) blocksize;

I think you can just pass in "int blocksize" right?

and 

ret = manager->discard(fs->io, start, count, fs->blocksize);

-Eric

> 	struct unix_private_data *data;
> 	errcode_t	retval = 0;
> 	__uint64_t	range[2];
> 
> 	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> 	data = (struct unix_private_data *) channel->private_data;
> 	EXT2_CHECK_MAGIC(data, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
> 
> 	if (!blocksize)
> 		return EXT2_ET_INVALID_ARGUMENT;
> 
> 	range[0] = (__uint64_t)(block);
> 	range[1] = (__uint64_t)(count);
> 	range[0] *= (__uint64_t)(*bsize);
> 	range[1] *= (__uint64_t)(*bsize);
> 
> 	return ioctl(data->dev, BLKDISCARD, &range);
> }
> 
> then we can call it like this:
> 
> ret = manager->discard(fs->io, start, count, &fs->blocksize);
> 
> Thanks a lot for reviewing this!
> 
> -Lukas
> 
>>
>> -Eric
>>
>>> +	return ioctl(u_priv->dev, BLKDISCARD, &range);
>>> +}
>>> +
>>> +#else
>>> +#define unix_discard(channel, block, count)	EXT2_ET_UNIMPLEMENTED
>>> +#endif
>>
>>
> 

Re: [PATCH 1/7] e2fsprogs: Add discard function into struct_io_manager

[Lukas Czerner]

On Tue, 16 Nov 2010, Eric Sandeen wrote:

-snip-

> >>> +
> >>> +	struct struct_ext2_filsys *fs;
> >>> +	struct unix_private_data *u_priv;
> >>> +	errcode_t	retval = 0;
> >>> +	__uint64_t	range[2];
> >>> +	int		blocksize;
> >>> +
> >>> +	EXT2_CHECK_MAGIC(channel, EXT2_ET_MAGIC_IO_CHANNEL);
> >>> +	u_priv = (struct unix_private_data *) channel->private_data;
> >>> +	EXT2_CHECK_MAGIC(u_priv, EXT2_ET_MAGIC_UNIX_IO_CHANNEL);
> >>> +
> >>> +	fs = (struct struct_ext2_filsys *) data;
> >>> +	if (!fs)
> >>> +		return EXT2_ET_INVALID_ARGUMENT;
> >>> +
> >>> +	blocksize = EXT2_BLOCK_SIZE(fs->super);
> >>
> >> This seems a little convoluted; you pass in *data, which gets you the fs,
> >> from which you get the super, from which you get the blocksize,
> >> which is all that you ever actually use here:
> >>
> >>> +	range[0] = (__uint64_t)(block);
> >>> +	range[1] = (__uint64_t)(count);
> >>> +	range[0] *= (__uint64_t)(blocksize);
> >>> +	range[1] *= (__uint64_t)(blocksize);
> >>
> >> any reason to not just pass in the blocksize?
> >>
> >> Maybe this is for flexibility for not-linux, but I guess we don't know what
> >> they need anyway...?
> >>
> >> And if you do that you can change "u_priv" to "data" just to match the
> >> other handlers, perhaps.
> > 
> > You're right, I was so closely following the example that I forget that
> > it can be done more simply. Does this looks good to you ?
> > 
> > static errcode_t unix_discard(io_channel channel, unsigned long long block,
> > 			     unsigned long long count, const void *blocksize)
> > {
> > 	unsigned int    *bsize = (unsigned int *) blocksize;
> 
> I think you can just pass in "int blocksize" right?
> 
> and 
> 
> ret = manager->discard(fs->io, start, count, fs->blocksize);
> 
> -Eric
> 

You are probably right, there is no need for io_manager to pass other
information than just block, count and blocksize, so I'll change discard
definition for that.

Thanks!

-Lukas

[PATCH 2/7] e2fsprogs: Add discard_zeroes_data into struct_io_manager

[Lukas Czerner]

When the device have discard support and simultaneously discard zeroes
data (and it is properly advertised), then we can take advantage of such
behavior in several e2fsprogs tools.

Add new variable discard_zeroes_data into struct_io_manager structure so
each io_manager can take advantage of this. Set it to 0 by default in
unix_io_manager and test_io_manager and fill it with BLKDISCARDZEROES
ioctl in unix_open.

Every other io_manager which would like to take advantage of this should
set the default to 0 in its io_manager definition and then check and set
the value preferably in *_open() function.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 lib/ext2fs/ext2_io.h |    1 +
 lib/ext2fs/test_io.c |    1 +
 lib/ext2fs/unix_io.c |   12 ++++++++++++
 3 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
index d202007..cc79fda 100644
--- a/lib/ext2fs/ext2_io.h
+++ b/lib/ext2fs/ext2_io.h
@@ -85,6 +85,7 @@ struct struct_io_manager {
 					int count, const void *data);
 	errcode_t (*discard)(io_channel channel, unsigned long long block,
 			     unsigned long long count, const void *data);
+	int	discard_zeroes_data;
 	long	reserved[16];
 };
 
diff --git a/lib/ext2fs/test_io.c b/lib/ext2fs/test_io.c
index 8d887a8..2fac849 100644
--- a/lib/ext2fs/test_io.c
+++ b/lib/ext2fs/test_io.c
@@ -89,6 +89,7 @@ static struct struct_io_manager struct_test_manager = {
 	test_get_stats,
 	test_read_blk64,
 	test_write_blk64,
+	0,				/* discard zeroes data */
 };
 
 io_manager test_io_manager = &struct_test_manager;
diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
index 5b6cdec..b3908dc 100644
--- a/lib/ext2fs/unix_io.c
+++ b/lib/ext2fs/unix_io.c
@@ -133,6 +133,7 @@ static struct struct_io_manager struct_unix_manager = {
 	unix_read_blk64,
 	unix_write_blk64,
 	unix_discard,
+	0,				/* discard zeroes data */
 };
 
 io_manager unix_io_manager = &struct_unix_manager;
@@ -425,6 +426,12 @@ static errcode_t flush_cached_blocks(io_channel channel,
 }
 #endif /* NO_IO_CACHE */
 
+#ifdef __linux__
+#ifndef BLKDISCARDZEROES
+#define BLKDISCARDZEROES _IO(0x12,124)
+#endif
+#endif
+
 static errcode_t unix_open(const char *name, int flags, io_channel *channel)
 {
 	io_channel	io = NULL;
@@ -487,6 +494,11 @@ static errcode_t unix_open(const char *name, int flags, io_channel *channel)
 	}
 #endif
 
+#ifdef BLKDISCARDZEROES
+	ioctl(data->dev, BLKDISCARDZEROES,
+		&unix_io_manager->discard_zeroes_data);
+#endif
+
 #if defined(__CYGWIN__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
 	/*
 	 * Some operating systems require that the buffers be aligned,
-- 
1.7.2.3

Re: [PATCH 2/7] e2fsprogs: Add discard_zeroes_data into struct_io_manager

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> When the device have discard support and simultaneously discard zeroes
> data (and it is properly advertised), then we can take advantage of such
> behavior in several e2fsprogs tools.
> 
> Add new variable discard_zeroes_data into struct_io_manager structure so
> each io_manager can take advantage of this. Set it to 0 by default in
> unix_io_manager and test_io_manager and fill it with BLKDISCARDZEROES
> ioctl in unix_open.
> 
> Every other io_manager which would like to take advantage of this should
> set the default to 0 in its io_manager definition and then check and set
> the value preferably in *_open() function.

Hm, sticking this in struct_io_manager seems a little bit odd.  Wrong,
even ;).

Maybe this can go in the flags on the fs->io_channel?

-Eric

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  lib/ext2fs/ext2_io.h |    1 +
>  lib/ext2fs/test_io.c |    1 +
>  lib/ext2fs/unix_io.c |   12 ++++++++++++
>  3 files changed, 14 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/ext2fs/ext2_io.h b/lib/ext2fs/ext2_io.h
> index d202007..cc79fda 100644
> --- a/lib/ext2fs/ext2_io.h
> +++ b/lib/ext2fs/ext2_io.h
> @@ -85,6 +85,7 @@ struct struct_io_manager {
>  					int count, const void *data);
>  	errcode_t (*discard)(io_channel channel, unsigned long long block,
>  			     unsigned long long count, const void *data);
> +	int	discard_zeroes_data;
>  	long	reserved[16];
>  };
>  
> diff --git a/lib/ext2fs/test_io.c b/lib/ext2fs/test_io.c
> index 8d887a8..2fac849 100644
> --- a/lib/ext2fs/test_io.c
> +++ b/lib/ext2fs/test_io.c
> @@ -89,6 +89,7 @@ static struct struct_io_manager struct_test_manager = {
>  	test_get_stats,
>  	test_read_blk64,
>  	test_write_blk64,
> +	0,				/* discard zeroes data */
>  };
>  
>  io_manager test_io_manager = &struct_test_manager;
> diff --git a/lib/ext2fs/unix_io.c b/lib/ext2fs/unix_io.c
> index 5b6cdec..b3908dc 100644
> --- a/lib/ext2fs/unix_io.c
> +++ b/lib/ext2fs/unix_io.c
> @@ -133,6 +133,7 @@ static struct struct_io_manager struct_unix_manager = {
>  	unix_read_blk64,
>  	unix_write_blk64,
>  	unix_discard,
> +	0,				/* discard zeroes data */
>  };
>  
>  io_manager unix_io_manager = &struct_unix_manager;
> @@ -425,6 +426,12 @@ static errcode_t flush_cached_blocks(io_channel channel,
>  }
>  #endif /* NO_IO_CACHE */
>  
> +#ifdef __linux__
> +#ifndef BLKDISCARDZEROES
> +#define BLKDISCARDZEROES _IO(0x12,124)
> +#endif
> +#endif
> +
>  static errcode_t unix_open(const char *name, int flags, io_channel *channel)
>  {
>  	io_channel	io = NULL;
> @@ -487,6 +494,11 @@ static errcode_t unix_open(const char *name, int flags, io_channel *channel)
>  	}
>  #endif
>  
> +#ifdef BLKDISCARDZEROES
> +	ioctl(data->dev, BLKDISCARDZEROES,
> +		&unix_io_manager->discard_zeroes_data);
> +#endif
> +
>  #if defined(__CYGWIN__) || defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
>  	/*
>  	 * Some operating systems require that the buffers be aligned,

[PATCH 3/7] e2fsck: Keep track of problems during the check

[Lukas Czerner]

It might be useful to know whether or not there has been any problem
during the check. This commit add new variable problem_history into
e2fsck_struct in order to store error codes.

When problem occur and fix_problem() is invoked, problem code is stored
into problem_history. Note that only real problem are stored, hence
problems with PR_NO_PROBLEM are ignored. This commit also adds
PR_NO_PROBLEM flag into problem for informative purposes only.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/e2fsck.h   |    1 +
 e2fsck/problem.c  |   31 +++++++++++++++++++++----------
 e2fsck/problem.h  |    3 +++
 e2fsck/problemP.h |    1 +
 4 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index d4df5f3..a6563cc 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -216,6 +216,7 @@ struct e2fsck_struct {
 	blk64_t	num_blocks;	/* Total number of blocks */
 	int	mount_flags;
 	blkid_cache blkid;	/* blkid cache */
+	__u32 problem_history;	/* store previous problem codes */
 
 #ifdef HAVE_SETJMP_H
 	jmp_buf	abort_loc;
diff --git a/e2fsck/problem.c b/e2fsck/problem.c
index 8032fda..9b987b0 100644
--- a/e2fsck/problem.c
+++ b/e2fsck/problem.c
@@ -407,7 +407,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 1: Checking inodes, blocks, and sizes */
 	{ PR_1_PASS_HEADER,
 	  N_("Pass 1: Checking @is, @bs, and sizes\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Root directory is not an inode */
 	{ PR_1_ROOT_NO_DIR, N_("@r is not a @d.  "),
@@ -896,7 +896,7 @@ static struct e2fsck_problem problem_table[] = {
 	{ PR_1B_PASS_HEADER,
 	  N_("\nRunning additional passes to resolve @bs claimed by more than one @i...\n"
 	  "Pass 1B: Rescanning for @m @bs\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Duplicate/bad block(s) header */
 	{ PR_1B_DUP_BLOCK_HEADER,
@@ -937,13 +937,13 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 1C: Scan directories for inodes with multiply-claimed blocks. */
 	{ PR_1C_PASS_HEADER,
 	  N_("Pass 1C: Scanning directories for @is with @m @bs\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 
 	/* Pass 1D: Reconciling multiply-claimed blocks */
 	{ PR_1D_PASS_HEADER,
 	  N_("Pass 1D: Reconciling @m @bs\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* File has duplicate blocks */
 	{ PR_1D_DUP_FILE,
@@ -988,7 +988,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 2: Checking directory structure */
 	{ PR_2_PASS_HEADER,
 	  N_("Pass 2: Checking @d structure\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Bad inode number for '.' */
 	{ PR_2_BAD_INODE_DOT,
@@ -1318,7 +1318,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 3: Checking directory connectivity */
 	{ PR_3_PASS_HEADER,
 	  N_("Pass 3: Checking @d connectivity\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Root inode not allocated */
 	{ PR_3_NO_ROOT_INODE,
@@ -1440,7 +1440,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 3A: Optimizing directories */
 	{ PR_3A_PASS_HEADER,
 	  N_("Pass 3A: Optimizing directories\n"),
-	  PROMPT_NONE, PR_PREEN_NOMSG },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Error iterating over directories */
 	{ PR_3A_OPTIMIZE_ITER,
@@ -1455,7 +1455,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Rehashing dir header */
 	{ PR_3A_OPTIMIZE_DIR_HEADER,
 	  N_("Optimizing directories: "),
-	  PROMPT_NONE, PR_MSG_ONLY },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Rehashing directory %d */
 	{ PR_3A_OPTIMIZE_DIR,
@@ -1472,7 +1472,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 4: Checking reference counts */
 	{ PR_4_PASS_HEADER,
 	  N_("Pass 4: Checking reference counts\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Unattached zero-length inode */
 	{ PR_4_ZERO_LEN_INODE,
@@ -1501,7 +1501,7 @@ static struct e2fsck_problem problem_table[] = {
 	/* Pass 5: Checking group summary information */
 	{ PR_5_PASS_HEADER,
 	  N_("Pass 5: Checking @g summary information\n"),
-	  PROMPT_NONE, 0 },
+	  PROMPT_NONE, PR_NO_PROBLEM },
 
 	/* Padding at end of inode bitmap is not set. */
 	{ PR_5_INODE_BMAP_PADDING,
@@ -1629,6 +1629,13 @@ static struct e2fsck_problem problem_table[] = {
 	  N_("@g %g @i(s) in use but @g is marked INODE_UNINIT\n"),
 	  PROMPT_FIX, PR_PREEN_OK },
 
+	{ PR_5_DONT_DISCARD,
+	  N_("There has been some errors during the check. I will not "
+	     "attempt to discard free data and inode blocks for safety "
+	     "reasons. If you wish to discard free data and inode blocks, "
+	     "run e2fsck once again.\n"),
+	  PROMPT_NONE, PR_NO_PROBLEM },
+
 	/* Post-Pass 5 errors */
 
 	/* Recreate journal if E2F_FLAG_JOURNAL_INODE flag is set */
@@ -1755,6 +1762,10 @@ int fix_problem(e2fsck_t ctx, problem_t code, struct problem_context *pctx)
 		printf(_("Unhandled error code (0x%x)!\n"), code);
 		return 0;
 	}
+	/* Keep track of real problems only */
+	if (!(ptr->flags & PR_NO_PROBLEM)) {
+		ctx->problem_history |= code;
+	}
 	if (!(ptr->flags & PR_CONFIG)) {
 		char	key[9], *new_desc;
 
diff --git a/e2fsck/problem.h b/e2fsck/problem.h
index 7c4c156..c3431ac 100644
--- a/e2fsck/problem.h
+++ b/e2fsck/problem.h
@@ -983,6 +983,9 @@ struct problem_context {
 /* Inode in use but group is marked INODE_UNINIT */
 #define PR_5_INODE_UNINIT		0x050019
 
+/* Do not attempt to discard */
+#define PR_5_DONT_DISCARD		0x05001A
+
 /*
  * Post-Pass 5 errors
  */
diff --git a/e2fsck/problemP.h b/e2fsck/problemP.h
index 6161189..ee6c1a3 100644
--- a/e2fsck/problemP.h
+++ b/e2fsck/problemP.h
@@ -41,3 +41,4 @@ struct latch_descr {
 #define PR_PREEN_NOHDR	0x040000 /* Don't print the preen header */
 #define PR_CONFIG	0x080000 /* This problem has been customized
 				    from the config file */
+#define PR_NO_PROBLEM	0x100000 /* Use No as an answer if preening */
-- 
1.7.2.3

Re: [PATCH 3/7] e2fsck: Keep track of problems during the check

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> It might be useful to know whether or not there has been any problem
> during the check. This commit add new variable problem_history into
> e2fsck_struct in order to store error codes.
> 
> When problem occur and fix_problem() is invoked, problem code is stored
> into problem_history. Note that only real problem are stored, hence
> problems with PR_NO_PROBLEM are ignored. This commit also adds
> PR_NO_PROBLEM flag into problem for informative purposes only.

Unless I'm missing something, don't we already keep track of
whether we did any repairs?  Based on the exit code from e2fsck;
per the man page:

EXIT CODE
       The exit code returned by e2fsck is the sum of the following conditions:
            0    - No errors
            1    - File system errors corrected
            2    - File system errors corrected, system should
                   be rebooted
...

etc.

-Eric

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  e2fsck/e2fsck.h   |    1 +
>  e2fsck/problem.c  |   31 +++++++++++++++++++++----------
>  e2fsck/problem.h  |    3 +++
>  e2fsck/problemP.h |    1 +
>  4 files changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index d4df5f3..a6563cc 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -216,6 +216,7 @@ struct e2fsck_struct {
>  	blk64_t	num_blocks;	/* Total number of blocks */
>  	int	mount_flags;
>  	blkid_cache blkid;	/* blkid cache */
> +	__u32 problem_history;	/* store previous problem codes */
>  
>  #ifdef HAVE_SETJMP_H
>  	jmp_buf	abort_loc;
> diff --git a/e2fsck/problem.c b/e2fsck/problem.c
> index 8032fda..9b987b0 100644
> --- a/e2fsck/problem.c
> +++ b/e2fsck/problem.c
> @@ -407,7 +407,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 1: Checking inodes, blocks, and sizes */
>  	{ PR_1_PASS_HEADER,
>  	  N_("Pass 1: Checking @is, @bs, and sizes\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Root directory is not an inode */
>  	{ PR_1_ROOT_NO_DIR, N_("@r is not a @d.  "),
> @@ -896,7 +896,7 @@ static struct e2fsck_problem problem_table[] = {
>  	{ PR_1B_PASS_HEADER,
>  	  N_("\nRunning additional passes to resolve @bs claimed by more than one @i...\n"
>  	  "Pass 1B: Rescanning for @m @bs\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Duplicate/bad block(s) header */
>  	{ PR_1B_DUP_BLOCK_HEADER,
> @@ -937,13 +937,13 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 1C: Scan directories for inodes with multiply-claimed blocks. */
>  	{ PR_1C_PASS_HEADER,
>  	  N_("Pass 1C: Scanning directories for @is with @m @bs\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  
>  	/* Pass 1D: Reconciling multiply-claimed blocks */
>  	{ PR_1D_PASS_HEADER,
>  	  N_("Pass 1D: Reconciling @m @bs\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* File has duplicate blocks */
>  	{ PR_1D_DUP_FILE,
> @@ -988,7 +988,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 2: Checking directory structure */
>  	{ PR_2_PASS_HEADER,
>  	  N_("Pass 2: Checking @d structure\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Bad inode number for '.' */
>  	{ PR_2_BAD_INODE_DOT,
> @@ -1318,7 +1318,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 3: Checking directory connectivity */
>  	{ PR_3_PASS_HEADER,
>  	  N_("Pass 3: Checking @d connectivity\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Root inode not allocated */
>  	{ PR_3_NO_ROOT_INODE,
> @@ -1440,7 +1440,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 3A: Optimizing directories */
>  	{ PR_3A_PASS_HEADER,
>  	  N_("Pass 3A: Optimizing directories\n"),
> -	  PROMPT_NONE, PR_PREEN_NOMSG },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Error iterating over directories */
>  	{ PR_3A_OPTIMIZE_ITER,
> @@ -1455,7 +1455,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Rehashing dir header */
>  	{ PR_3A_OPTIMIZE_DIR_HEADER,
>  	  N_("Optimizing directories: "),
> -	  PROMPT_NONE, PR_MSG_ONLY },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Rehashing directory %d */
>  	{ PR_3A_OPTIMIZE_DIR,
> @@ -1472,7 +1472,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 4: Checking reference counts */
>  	{ PR_4_PASS_HEADER,
>  	  N_("Pass 4: Checking reference counts\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Unattached zero-length inode */
>  	{ PR_4_ZERO_LEN_INODE,
> @@ -1501,7 +1501,7 @@ static struct e2fsck_problem problem_table[] = {
>  	/* Pass 5: Checking group summary information */
>  	{ PR_5_PASS_HEADER,
>  	  N_("Pass 5: Checking @g summary information\n"),
> -	  PROMPT_NONE, 0 },
> +	  PROMPT_NONE, PR_NO_PROBLEM },
>  
>  	/* Padding at end of inode bitmap is not set. */
>  	{ PR_5_INODE_BMAP_PADDING,
> @@ -1629,6 +1629,13 @@ static struct e2fsck_problem problem_table[] = {
>  	  N_("@g %g @i(s) in use but @g is marked INODE_UNINIT\n"),
>  	  PROMPT_FIX, PR_PREEN_OK },
>  
> +	{ PR_5_DONT_DISCARD,
> +	  N_("There has been some errors during the check. I will not "
> +	     "attempt to discard free data and inode blocks for safety "
> +	     "reasons. If you wish to discard free data and inode blocks, "
> +	     "run e2fsck once again.\n"),
> +	  PROMPT_NONE, PR_NO_PROBLEM },
> +
>  	/* Post-Pass 5 errors */
>  
>  	/* Recreate journal if E2F_FLAG_JOURNAL_INODE flag is set */
> @@ -1755,6 +1762,10 @@ int fix_problem(e2fsck_t ctx, problem_t code, struct problem_context *pctx)
>  		printf(_("Unhandled error code (0x%x)!\n"), code);
>  		return 0;
>  	}
> +	/* Keep track of real problems only */
> +	if (!(ptr->flags & PR_NO_PROBLEM)) {
> +		ctx->problem_history |= code;
> +	}
>  	if (!(ptr->flags & PR_CONFIG)) {
>  		char	key[9], *new_desc;
>  
> diff --git a/e2fsck/problem.h b/e2fsck/problem.h
> index 7c4c156..c3431ac 100644
> --- a/e2fsck/problem.h
> +++ b/e2fsck/problem.h
> @@ -983,6 +983,9 @@ struct problem_context {
>  /* Inode in use but group is marked INODE_UNINIT */
>  #define PR_5_INODE_UNINIT		0x050019
>  
> +/* Do not attempt to discard */
> +#define PR_5_DONT_DISCARD		0x05001A
> +
>  /*
>   * Post-Pass 5 errors
>   */
> diff --git a/e2fsck/problemP.h b/e2fsck/problemP.h
> index 6161189..ee6c1a3 100644
> --- a/e2fsck/problemP.h
> +++ b/e2fsck/problemP.h
> @@ -41,3 +41,4 @@ struct latch_descr {
>  #define PR_PREEN_NOHDR	0x040000 /* Don't print the preen header */
>  #define PR_CONFIG	0x080000 /* This problem has been customized
>  				    from the config file */
> +#define PR_NO_PROBLEM	0x100000 /* Use No as an answer if preening */

[PATCH 4/7] e2fsck: Discard free data and inode blocks.

[Lukas Czerner]

In Pass 5 when we are checking block and inode bitmaps we have great
opportunity to discard free space and unused inodes on the device,
because bitmaps has just been verified as valid. This commit takes
advantage of this opportunity and discards both, all free space and
unused inodes.

I have added new set of options, 'nodiscard' and 'discard'. When the
underlying devices does not support discard, or discard ends with an
error, or when any kind of error occurs on the filesystem, no further
discard attempt will be made and the e2fsck will behave as it would
with nodiscard option provided.

As an addition, when there is any not-yet-zeroed inode table and
discard zeroes data, then inode table is marked as zeroed.

This is off by default.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 e2fsck/e2fsck.8.in |   14 +++++++++
 e2fsck/e2fsck.h    |    1 +
 e2fsck/pass5.c     |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 e2fsck/unix.c      |   10 ++++++-
 4 files changed, 105 insertions(+), 1 deletions(-)

diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
index 3fb15e6..2da253a 100644
--- a/e2fsck/e2fsck.8.in
+++ b/e2fsck/e2fsck.8.in
@@ -189,6 +189,20 @@ be 1 or 2.  The default extended attribute version format is 2.
 .BI fragcheck
 During pass 1, print a detailed report of any discontiguous blocks for
 files in the filesystem.
+.TP
+.BI discard
+Discard, attempt to discard free blocks and unused inode blocks after the full
+filesystem check (discardding blocks is useful on solid state devices and sparse
+/ thin-provisioned storage). Note that discard is done in pass 5 AFTER the
+filesystem has been fully checked and does not contains recognizable errors.
+However there might be cases where
+.B e2fsck
+does not fully recognise the problem and hence in this case this
+option may prevent you from further manual data recovery.
+.TP
+.BI nodiscard
+Do not attempt to discard free blocks and unused inode blocks. This option is
+exacly the oposite of discard option. This is set as default.
 .RE
 .TP
 .B \-f
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index a6563cc..65601ab 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -155,6 +155,7 @@ struct resource_track {
 #define E2F_OPT_WRITECHECK	0x0200
 #define E2F_OPT_COMPRESS_DIRS	0x0400
 #define E2F_OPT_FRAGCHECK	0x0800
+#define E2F_OPT_DISCARD		0x1000
 
 /*
  * E2fsck flags
diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
index cbc12f3..3819901 100644
--- a/e2fsck/pass5.c
+++ b/e2fsck/pass5.c
@@ -10,9 +10,18 @@
  *
  */
 
+#include <stdint.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <errno.h>
+
 #include "e2fsck.h"
 #include "problem.h"
 
+#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
+
 static void check_block_bitmaps(e2fsck_t ctx);
 static void check_inode_bitmaps(e2fsck_t ctx);
 static void check_inode_end(e2fsck_t ctx);
@@ -39,6 +48,12 @@ void e2fsck_pass5(e2fsck_t ctx)
 		if ((ctx->progress)(ctx, 5, 0, ctx->fs->group_desc_count*2))
 			return;
 
+	if ((ctx->options & E2F_OPT_DISCARD) &&
+	    (ctx->problem_history)) {
+		fix_problem(ctx, PR_5_DONT_DISCARD, &pctx);
+		ctx->options &= ~E2F_OPT_DISCARD;
+	}
+
 	e2fsck_read_bitmaps(ctx);
 
 	check_block_bitmaps(ctx);
@@ -64,6 +79,19 @@ void e2fsck_pass5(e2fsck_t ctx)
 	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
 }
 
+static void e2fsck_should_discard(e2fsck_t ctx, io_manager manager,
+				  blk64_t start, blk64_t count)
+{
+	ext2_filsys fs = ctx->fs;
+	int ret = 0;
+
+	if (ctx->options & E2F_OPT_DISCARD)
+		ret = manager->discard(fs->io, start, count, fs);
+
+	if (ret)
+		ctx->options &= ~E2F_OPT_DISCARD;
+}
+
 #define NO_BLK ((blk64_t) -1)
 
 static void print_bitmap_problem(e2fsck_t ctx, int problem,
@@ -108,6 +136,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	group = 0;
 	int	blocks = 0;
 	blk64_t	free_blocks = 0;
+	blk64_t first_free = ext2fs_blocks_count(fs->super);
 	int	group_free = 0;
 	int	actual, bitmap;
 	struct problem_context	pctx;
@@ -120,6 +149,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
 	int	cmp_block = 0;
 	int	redo_flag = 0;
 	blk64_t	super_blk, old_desc_blk, new_desc_blk;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -280,11 +310,24 @@ redo_counts:
 		}
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
+		/*
+		 * If there a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
 
 	do_counts:
 		if (!bitmap && (!skip_group || csum_flag)) {
 			group_free++;
 			free_blocks++;
+			if (first_free > i)
+				first_free = i;
+		} else {
+			if ((i > first_free) &&
+			   (ctx->options & E2F_OPT_DISCARD))
+				e2fsck_should_discard(ctx, manager, first_free,
+						      (i - first_free));
+			first_free = ext2fs_blocks_count(fs->super);
 		}
 		blocks ++;
 		if ((blocks == fs->super->s_blocks_per_group) ||
@@ -381,6 +424,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
 	int		csum_flag;
 	int		skip_group = 0;
 	int		redo_flag = 0;
+	io_manager	manager = ctx->fs->io->manager;
 
 	clear_problem_context(&pctx);
 	free_array = (int *) e2fsck_allocate_memory(ctx,
@@ -500,6 +544,11 @@ redo_counts:
 		}
 		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
 		had_problem++;
+		/*
+		 * If there is a problem we should turn off the discard so we
+		 * do not compromise the filesystem.
+		 */
+		ctx->options &= ~E2F_OPT_DISCARD;
 
 do_counts:
 		if (bitmap) {
@@ -509,11 +558,43 @@ do_counts:
 			group_free++;
 			free_inodes++;
 		}
+
 		inodes++;
 		if ((inodes == fs->super->s_inodes_per_group) ||
 		    (i == fs->super->s_inodes_count)) {
+
 			free_array[group] = group_free;
 			dir_array[group] = dirs_count;
+
+			/* Discard inode table */
+			if (ctx->options & E2F_OPT_DISCARD) {
+				blk64_t used_blks, blk, num;
+
+				used_blks = DIV_ROUND_UP(
+					(EXT2_INODES_PER_GROUP(fs->super) -
+					group_free),
+					EXT2_INODES_PER_BLOCK(fs->super));
+
+				blk = ext2fs_inode_table_loc(fs, group) +
+				      used_blks;
+				num = fs->inode_blocks_per_group -
+				      used_blks;
+				e2fsck_should_discard(ctx, manager, blk, num);
+			}
+
+			/*
+			 * If discard zeroes data and the group inode table
+			 * was not zeroed yet, set itable as zeroed
+			 */
+			if ((ctx->options & E2F_OPT_DISCARD) &&
+			    (manager->discard_zeroes_data) &&
+			    !(ext2fs_bg_flags_test(fs, group,
+						  EXT2_BG_INODE_ZEROED))) {
+				ext2fs_bg_flags_set(fs, group,
+						    EXT2_BG_INODE_ZEROED);
+				ext2fs_group_desc_csum_set(fs, group);
+			}
+
 			group ++;
 			inodes = 0;
 			skip_group = 0;
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index 7eb269c..4cf55a9 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		} else if (strcmp(token, "fragcheck") == 0) {
 			ctx->options |= E2F_OPT_FRAGCHECK;
 			continue;
+		} else if (strcmp(token, "discard") == 0) {
+			ctx->options |= E2F_OPT_DISCARD;
+			continue;
+		} else if (strcmp(token, "nodiscard") == 0) {
+			ctx->options &= ~E2F_OPT_DISCARD;
+			continue;
 		} else {
 			fprintf(stderr, _("Unknown extended option: %s\n"),
 				token);
@@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
 		       "Valid extended options are:\n"), stderr);
 		fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
 		fputs(("\tfragcheck\n"), stderr);
+		fputs(("\tdiscard\n"), stderr);
+		fputs(("\tnodiscard\n"), stderr);
 		fputc('\n', stderr);
 		exit(1);
 	}
@@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
 		ctx->program_name = *argv;
 	else
 		ctx->program_name = "e2fsck";
+
 	while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
 		switch (c) {
 		case 'C':
@@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
 	return retval;
 }
 

Re: [PATCH 4/7] e2fsck: Discard free data and inode blocks.

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> In Pass 5 when we are checking block and inode bitmaps we have great
> opportunity to discard free space and unused inodes on the device,
> because bitmaps has just been verified as valid. This commit takes
> advantage of this opportunity and discards both, all free space and
> unused inodes.
> 
> I have added new set of options, 'nodiscard' and 'discard'. When the
> underlying devices does not support discard, or discard ends with an
> error, or when any kind of error occurs on the filesystem, no further
> discard attempt will be made and the e2fsck will behave as it would
> with nodiscard option provided.
> 
> As an addition, when there is any not-yet-zeroed inode table and
> discard zeroes data, then inode table is marked as zeroed.
> 
> This is off by default.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  e2fsck/e2fsck.8.in |   14 +++++++++
>  e2fsck/e2fsck.h    |    1 +
>  e2fsck/pass5.c     |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  e2fsck/unix.c      |   10 ++++++-
>  4 files changed, 105 insertions(+), 1 deletions(-)
> 
> diff --git a/e2fsck/e2fsck.8.in b/e2fsck/e2fsck.8.in
> index 3fb15e6..2da253a 100644
> --- a/e2fsck/e2fsck.8.in
> +++ b/e2fsck/e2fsck.8.in
> @@ -189,6 +189,20 @@ be 1 or 2.  The default extended attribute version format is 2.
>  .BI fragcheck
>  During pass 1, print a detailed report of any discontiguous blocks for
>  files in the filesystem.
> +.TP
> +.BI discard
> +Discard, attempt to discard free blocks and unused inode blocks after the full

No need to restate "Discard, " - s/Discard, a/A/

> +filesystem check (discardding blocks is useful on solid state devices and sparse

discarding (only 1 d in the middle)

> +/ thin-provisioned storage). Note that discard is done in pass 5 AFTER the
> +filesystem has been fully checked and does not contains recognizable errors.

maybe "and only if it does not contain ..."

> +However there might be cases where
> +.B e2fsck
> +does not fully recognise the problem and hence in this case this

maybe "recognize a problem" ("the problem" seems like you refer to
some specific problem)

> +option may prevent you from further manual data recovery.
> +.TP
> +.BI nodiscard
> +Do not attempt to discard free blocks and unused inode blocks. This option is
> +exacly the oposite of discard option. This is set as default.

"exactly the opposite."

I wonder if we really need an option to restate the default, but it doesn't
matter much to me.

>  .RE
>  .TP
>  .B \-f
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index a6563cc..65601ab 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -155,6 +155,7 @@ struct resource_track {
>  #define E2F_OPT_WRITECHECK	0x0200
>  #define E2F_OPT_COMPRESS_DIRS	0x0400
>  #define E2F_OPT_FRAGCHECK	0x0800
> +#define E2F_OPT_DISCARD		0x1000
>  
>  /*
>   * E2fsck flags
> diff --git a/e2fsck/pass5.c b/e2fsck/pass5.c
> index cbc12f3..3819901 100644
> --- a/e2fsck/pass5.c
> +++ b/e2fsck/pass5.c
> @@ -10,9 +10,18 @@
>   *
>   */
>  
> +#include <stdint.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
>  #include "e2fsck.h"
>  #include "problem.h"
>  
> +#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> +
>  static void check_block_bitmaps(e2fsck_t ctx);
>  static void check_inode_bitmaps(e2fsck_t ctx);
>  static void check_inode_end(e2fsck_t ctx);
> @@ -39,6 +48,12 @@ void e2fsck_pass5(e2fsck_t ctx)
>  		if ((ctx->progress)(ctx, 5, 0, ctx->fs->group_desc_count*2))
>  			return;
>  
> +	if ((ctx->options & E2F_OPT_DISCARD) &&
> +	    (ctx->problem_history)) {
> +		fix_problem(ctx, PR_5_DONT_DISCARD, &pctx);
> +		ctx->options &= ~E2F_OPT_DISCARD;
> +	}
> +

if you get rid of ->problem_history I think maybe you can use
(ctx->fs->flags & EXT2_FLAG_CHANGED) instead, or 
ext2fs_test_changed(ctx->fs).

Ted may be a better guide here though...

>  	e2fsck_read_bitmaps(ctx);
>  
>  	check_block_bitmaps(ctx);
> @@ -64,6 +79,19 @@ void e2fsck_pass5(e2fsck_t ctx)
>  	print_resource_track(ctx, _("Pass 5"), &rtrack, ctx->fs->io);
>  }
>  
> +static void e2fsck_should_discard(e2fsck_t ctx, io_manager manager,
> +				  blk64_t start, blk64_t count)
> +{
> +	ext2_filsys fs = ctx->fs;
> +	int ret = 0;
> +
> +	if (ctx->options & E2F_OPT_DISCARD)
> +		ret = manager->discard(fs->io, start, count, fs);

as suggested earlier maybe you can just pass in blocksize unless there's
a real expectation that other OSes might need other fs-> info?

"e2fsck_should_discard" sounds lke it is a test that would return
true/false, but this actually does discard and is a void.

I'd probably rename it to better imply what itdoes, maybe
just e2fsck_blocks_discard or e2fsck_discard ?

> +
> +	if (ret)

any point to issuing a warning here if we encounter an error and stop?

> +		ctx->options &= ~E2F_OPT_DISCARD;
> +}
> +
>  #define NO_BLK ((blk64_t) -1)
>  
>  static void print_bitmap_problem(e2fsck_t ctx, int problem,
> @@ -108,6 +136,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
>  	int	group = 0;
>  	int	blocks = 0;
>  	blk64_t	free_blocks = 0;
> +	blk64_t first_free = ext2fs_blocks_count(fs->super);
>  	int	group_free = 0;
>  	int	actual, bitmap;
>  	struct problem_context	pctx;
> @@ -120,6 +149,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
>  	int	cmp_block = 0;
>  	int	redo_flag = 0;
>  	blk64_t	super_blk, old_desc_blk, new_desc_blk;
> +	io_manager	manager = ctx->fs->io->manager;
>  
>  	clear_problem_context(&pctx);
>  	free_array = (int *) e2fsck_allocate_memory(ctx,
> @@ -280,11 +310,24 @@ redo_counts:
>  		}
>  		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
>  		had_problem++;
> +		/*
> +		 * If there a problem we should turn off the discard so we
> +		 * do not compromise the filesystem.
> +		 */
> +		ctx->options &= ~E2F_OPT_DISCARD;

Would a warning be reasonable here?

Or, maybe rather than trying to catch all the locations where
you want to turn off the flag, can we just test for a changed
fs prior to any discard in e2fsck_should_discard()?
Not sure, just thinking out loud.

>  	do_counts:
>  		if (!bitmap && (!skip_group || csum_flag)) {
>  			group_free++;
>  			free_blocks++;
> +			if (first_free > i)
> +				first_free = i;
> +		} else {
> +			if ((i > first_free) &&
> +			   (ctx->options & E2F_OPT_DISCARD))
> +				e2fsck_should_discard(ctx, manager, first_free,
> +						      (i - first_free));
> +			first_free = ext2fs_blocks_count(fs->super);
>  		}
>  		blocks ++;
>  		if ((blocks == fs->super->s_blocks_per_group) ||
> @@ -381,6 +424,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
>  	int		csum_flag;
>  	int		skip_group = 0;
>  	int		redo_flag = 0;
> +	io_manager	manager = ctx->fs->io->manager;
>  
>  	clear_problem_context(&pctx);
>  	free_array = (int *) e2fsck_allocate_memory(ctx,
> @@ -500,6 +544,11 @@ redo_counts:
>  		}
>  		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
>  		had_problem++;
> +		/*
> +		 * If there is a problem we should turn off the discard so we
> +		 * do not compromise the filesystem.
> +		 */
> +		ctx->options &= ~E2F_OPT_DISCARD;
>  
>  do_counts:
>  		if (bitmap) {
> @@ -509,11 +558,43 @@ do_counts:
>  			group_free++;
>  			free_inodes++;
>  		}
> +
>  		inodes++;
>  		if ((inodes == fs->super->s_inodes_per_group) ||
>  		    (i == fs->super->s_inodes_count)) {
> +
>  			free_array[group] = group_free;
>  			dir_array[group] = dirs_count;
> +
> +			/* Discard inode table */
> +			if (ctx->options & E2F_OPT_DISCARD) {
> +				blk64_t used_blks, blk, num;
> +
> +				used_blks = DIV_ROUND_UP(
> +					(EXT2_INODES_PER_GROUP(fs->super) -
> +					group_free),
> +					EXT2_INODES_PER_BLOCK(fs->super));
> +
> +				blk = ext2fs_inode_table_loc(fs, group) +
> +				      used_blks;
> +				num = fs->inode_blocks_per_group -
> +				      used_blks;
> +				e2fsck_should_discard(ctx, manager, blk, num);
> +			}
> +
> +			/*
> +			 * If discard zeroes data and the group inode table
> +			 * was not zeroed yet, set itable as zeroed
> +			 */
> +			if ((ctx->options & E2F_OPT_DISCARD) &&
> +			    (manager->discard_zeroes_data) &&
> +			    !(ext2fs_bg_flags_test(fs, group,
> +						  EXT2_BG_INODE_ZEROED))) {
> +				ext2fs_bg_flags_set(fs, group,
> +						    EXT2_BG_INODE_ZEROED);
> +				ext2fs_group_desc_csum_set(fs, group);
> +			}

Maybe for another patch, but even if we're not discarding, should we
be manually zeroing out here & setting the flag?  Hm I guess not,
that'd be slow and defeat the purpose wouldn't it...

> +
>  			group ++;
>  			inodes = 0;
>  			skip_group = 0;
> diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> index 7eb269c..4cf55a9 100644
> --- a/e2fsck/unix.c
> +++ b/e2fsck/unix.c
> @@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
>  		} else if (strcmp(token, "fragcheck") == 0) {
>  			ctx->options |= E2F_OPT_FRAGCHECK;
>  			continue;
> +		} else if (strcmp(token, "discard") == 0) {
> +			ctx->options |= E2F_OPT_DISCARD;
> +			continue;
> +		} else if (strcmp(token, "nodiscard") == 0) {
> +			ctx->options &= ~E2F_OPT_DISCARD;
> +			continue;
>  		} else {
>  			fprintf(stderr, _("Unknown extended option: %s\n"),
>  				token);
> @@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
>  		       "Valid extended options are:\n"), stderr);
>  		fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
>  		fputs(("\tfragcheck\n"), stderr);
> +		fputs(("\tdiscard\n"), stderr);
> +		fputs(("\tnodiscard\n"), stderr);
>  		fputc('\n', stderr);
>  		exit(1);
>  	}
> @@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
>  		ctx->program_name = *argv;
>  	else
>  		ctx->program_name = "e2fsck";
> +
>  	while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
>  		switch (c) {
>  		case 'C':
> @@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
>  	return retval;
>  }
>  
> -
>  static const char *my_ver_string = E2FSPROGS_VERSION;
>  static const char *my_ver_date = E2FSPROGS_DATE;
>  

Re: [PATCH 4/7] e2fsck: Discard free data and inode blocks.

[Lukas Czerner]

On Tue, 16 Nov 2010, Eric Sandeen wrote:

-snip-
> >  
> > +static void e2fsck_should_discard(e2fsck_t ctx, io_manager manager,
> > +				  blk64_t start, blk64_t count)
> > +{
> > +	ext2_filsys fs = ctx->fs;
> > +	int ret = 0;
> > +
> > +	if (ctx->options & E2F_OPT_DISCARD)
> > +		ret = manager->discard(fs->io, start, count, fs);
> 
> as suggested earlier maybe you can just pass in blocksize unless there's
> a real expectation that other OSes might need other fs-> info?
> 
> "e2fsck_should_discard" sounds lke it is a test that would return
> true/false, but this actually does discard and is a void.
> 
> I'd probably rename it to better imply what itdoes, maybe
> just e2fsck_blocks_discard or e2fsck_discard ?
> 
> > +
> > +	if (ret)
> 
> any point to issuing a warning here if we encounter an error and stop?

Right.

> 
> > +		ctx->options &= ~E2F_OPT_DISCARD;
> > +}
> > +
> >  #define NO_BLK ((blk64_t) -1)
> >  
> >  static void print_bitmap_problem(e2fsck_t ctx, int problem,
> > @@ -108,6 +136,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
> >  	int	group = 0;
> >  	int	blocks = 0;
> >  	blk64_t	free_blocks = 0;
> > +	blk64_t first_free = ext2fs_blocks_count(fs->super);
> >  	int	group_free = 0;
> >  	int	actual, bitmap;
> >  	struct problem_context	pctx;
> > @@ -120,6 +149,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
> >  	int	cmp_block = 0;
> >  	int	redo_flag = 0;
> >  	blk64_t	super_blk, old_desc_blk, new_desc_blk;
> > +	io_manager	manager = ctx->fs->io->manager;
> >  
> >  	clear_problem_context(&pctx);
> >  	free_array = (int *) e2fsck_allocate_memory(ctx,
> > @@ -280,11 +310,24 @@ redo_counts:
> >  		}
> >  		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
> >  		had_problem++;
> > +		/*
> > +		 * If there a problem we should turn off the discard so we
> > +		 * do not compromise the filesystem.
> > +		 */
> > +		ctx->options &= ~E2F_OPT_DISCARD;
> 
> Would a warning be reasonable here?

I am not really sure this is necessary.

> 
> Or, maybe rather than trying to catch all the locations where
> you want to turn off the flag, can we just test for a changed
> fs prior to any discard in e2fsck_should_discard()?
> Not sure, just thinking out loud.

Adding test for changed filesystem into e2fsck_should_discard() does
make sence, however it is not sufficient because even in case of fs
error it may hit discard before any changes to filesystem has been made.

> 
> >  	do_counts:
> >  		if (!bitmap && (!skip_group || csum_flag)) {
> >  			group_free++;
> >  			free_blocks++;
> > +			if (first_free > i)
> > +				first_free = i;
> > +		} else {
> > +			if ((i > first_free) &&
> > +			   (ctx->options & E2F_OPT_DISCARD))
> > +				e2fsck_should_discard(ctx, manager, first_free,
> > +						      (i - first_free));
> > +			first_free = ext2fs_blocks_count(fs->super);
> >  		}
> >  		blocks ++;
> >  		if ((blocks == fs->super->s_blocks_per_group) ||
> > @@ -381,6 +424,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
> >  	int		csum_flag;
> >  	int		skip_group = 0;
> >  	int		redo_flag = 0;
> > +	io_manager	manager = ctx->fs->io->manager;
> >  
> >  	clear_problem_context(&pctx);
> >  	free_array = (int *) e2fsck_allocate_memory(ctx,
> > @@ -500,6 +544,11 @@ redo_counts:
> >  		}
> >  		ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
> >  		had_problem++;
> > +		/*
> > +		 * If there is a problem we should turn off the discard so we
> > +		 * do not compromise the filesystem.
> > +		 */
> > +		ctx->options &= ~E2F_OPT_DISCARD;
> >  
> >  do_counts:
> >  		if (bitmap) {
> > @@ -509,11 +558,43 @@ do_counts:
> >  			group_free++;
> >  			free_inodes++;
> >  		}
> > +
> >  		inodes++;
> >  		if ((inodes == fs->super->s_inodes_per_group) ||
> >  		    (i == fs->super->s_inodes_count)) {
> > +
> >  			free_array[group] = group_free;
> >  			dir_array[group] = dirs_count;
> > +
> > +			/* Discard inode table */
> > +			if (ctx->options & E2F_OPT_DISCARD) {
> > +				blk64_t used_blks, blk, num;
> > +
> > +				used_blks = DIV_ROUND_UP(
> > +					(EXT2_INODES_PER_GROUP(fs->super) -
> > +					group_free),
> > +					EXT2_INODES_PER_BLOCK(fs->super));
> > +
> > +				blk = ext2fs_inode_table_loc(fs, group) +
> > +				      used_blks;
> > +				num = fs->inode_blocks_per_group -
> > +				      used_blks;
> > +				e2fsck_should_discard(ctx, manager, blk, num);
> > +			}
> > +
> > +			/*
> > +			 * If discard zeroes data and the group inode table
> > +			 * was not zeroed yet, set itable as zeroed
> > +			 */
> > +			if ((ctx->options & E2F_OPT_DISCARD) &&
> > +			    (manager->discard_zeroes_data) &&
> > +			    !(ext2fs_bg_flags_test(fs, group,
> > +						  EXT2_BG_INODE_ZEROED))) {
> > +				ext2fs_bg_flags_set(fs, group,
> > +						    EXT2_BG_INODE_ZEROED);
> > +				ext2fs_group_desc_csum_set(fs, group);
> > +			}
> 
> Maybe for another patch, but even if we're not discarding, should we
> be manually zeroing out here & setting the flag?  Hm I guess not,
> that'd be slow and defeat the purpose wouldn't it...
> 
> > +
> >  			group ++;
> >  			inodes = 0;
> >  			skip_group = 0;
> > diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> > index 7eb269c..4cf55a9 100644
> > --- a/e2fsck/unix.c
> > +++ b/e2fsck/unix.c
> > @@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
> >  		} else if (strcmp(token, "fragcheck") == 0) {
> >  			ctx->options |= E2F_OPT_FRAGCHECK;
> >  			continue;
> > +		} else if (strcmp(token, "discard") == 0) {
> > +			ctx->options |= E2F_OPT_DISCARD;
> > +			continue;
> > +		} else if (strcmp(token, "nodiscard") == 0) {
> > +			ctx->options &= ~E2F_OPT_DISCARD;
> > +			continue;
> >  		} else {
> >  			fprintf(stderr, _("Unknown extended option: %s\n"),
> >  				token);
> > @@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
> >  		       "Valid extended options are:\n"), stderr);
> >  		fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
> >  		fputs(("\tfragcheck\n"), stderr);
> > +		fputs(("\tdiscard\n"), stderr);
> > +		fputs(("\tnodiscard\n"), stderr);
> >  		fputc('\n', stderr);
> >  		exit(1);
> >  	}
> > @@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
> >  		ctx->program_name = *argv;
> >  	else
> >  		ctx->program_name = "e2fsck";
> > +
> >  	while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
> >  		switch (c) {
> >  		case 'C':
> > @@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
> >  	return retval;
> >  }
> >  
> > -
> >  static const char *my_ver_string = E2FSPROGS_VERSION;
> >  static const char *my_ver_date = E2FSPROGS_DATE;
> >  
> 
> 

-- 

[PATCH 5/7] mke2fs: Change -K option to discard/nodiscard

[Lukas Czerner]

It would be nice to have consistent "discard" options in every system
tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
and "nodiscard" is more descriptive instead of just "-K" and can be
easily defaulted and it is something we can not do with "-K".

With this commit you need to specify extended option like this:

./mke2fs -T <fstype> -E nodiscard <device>

in order make a filesystem without discarding the device first. And

./mke2fs -T <fstype> -E discard <device>

respectively.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.8.in |   18 +++++++++++-------
 misc/mke2fs.c    |   13 ++++++++-----
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index b46e7e2..6b437bc 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
 .I journal-options
 ]
 [
-.B \-K
-]
-[
 .B \-N
 .I number-of-inodes
 ]
@@ -240,6 +237,17 @@ enable lazy inode table initialization.
 .B test_fs
 Set a flag in the filesystem superblock indicating that it may be
 mounted using experimental kernel code, such as the ext4dev filesystem.
+.TP
+.BI discard
+Attempt to discard blocks at mkfs time (discarding blocks initially is useful
+on solid state devices and sparse / thin-provisioned storage). When the device
+advertise that discard also zeroes data (any subsequent read after the discard
+and before write returns zero), then mark all not-yet-zeroed inode table as
+zeroed. This significantly speeds up filesystem initialization. This is set
+as default.
+.TP
+.BI nodiscard
+Do not attempt to discard blocks at mkfs time.
 .RE
 .TP
 .BI \-f " fragment-size"
@@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
 @JDEV@.BR size " or " device
 @JDEV@options can be given for a filesystem.
 .TP
-.BI \-K
-Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
-is useful on solid state devices and sparse / thin-provisioned storage).
-.TP
 .BI \-l " filename"
 Read the bad blocks list from
 .IR filename .
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0980045..32e3a2b 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
 				lazy_itable_init = strtoul(arg, &p, 0);
 			else
 				lazy_itable_init = 1;
+		} else if (!strcmp(token, "discard")) {
+			discard = 1;
+		} else if (!strcmp(token, "nodiscard")) {
+			discard = 0;
 		} else {
 			r_usage++;
 			badopt = token;
@@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
 			"\tstripe-width=<RAID stride * data disks in blocks>\n"
 			"\tresize=<resize maximum size in blocks>\n"
 			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
-			"\ttest_fs\n\n"),
+			"\ttest_fs\n"
+			"\tdiscard\n"
+			"\tnodiscard\n\n"),
 			badopt ? badopt : "");
 		free(buf);
 		exit(1);
@@ -1168,7 +1174,7 @@ static void PRS(int argc, char *argv[])
 	}
 
 	while ((c = getopt (argc, argv,
-		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:KL:M:N:O:R:ST:U:V")) != EOF) {
+		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:L:M:N:O:R:ST:U:V")) != EOF) {
 		switch (c) {
 		case 'b':
 			blocksize = strtol(optarg, &tmp, 0);
@@ -1247,9 +1253,6 @@ static void PRS(int argc, char *argv[])
 		case 'J':
 			parse_journal_opts(optarg);
 			break;
-		case 'K':
-			discard = 0;
-			break;
 		case 'j':
 			if (!journal_size)
 				journal_size = -1;
-- 
1.7.2.3

Re: [PATCH 5/7] mke2fs: Change -K option to discard/nodiscard

[Eric Sandeen]

Lukas Czerner wrote:
> It would be nice to have consistent "discard" options in every system
> tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
> and "nodiscard" is more descriptive instead of just "-K" and can be
> easily defaulted and it is something we can not do with "-K".
> 
> With this commit you need to specify extended option like this:
> 
> ./mke2fs -T <fstype> -E nodiscard <device>
> 
> in order make a filesystem without discarding the device first. And
> 
> ./mke2fs -T <fstype> -E discard <device>
> 
> respectively.

Sadly, we probably need to map -K onto -E nodiscard and deprecate
it for now, rather than just removing it altogether?

-Eric

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  misc/mke2fs.8.in |   18 +++++++++++-------
>  misc/mke2fs.c    |   13 ++++++++-----
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
> index b46e7e2..6b437bc 100644
> --- a/misc/mke2fs.8.in
> +++ b/misc/mke2fs.8.in
> @@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
>  .I journal-options
>  ]
>  [
> -.B \-K
> -]
> -[
>  .B \-N
>  .I number-of-inodes
>  ]
> @@ -240,6 +237,17 @@ enable lazy inode table initialization.
>  .B test_fs
>  Set a flag in the filesystem superblock indicating that it may be
>  mounted using experimental kernel code, such as the ext4dev filesystem.
> +.TP
> +.BI discard
> +Attempt to discard blocks at mkfs time (discarding blocks initially is useful
> +on solid state devices and sparse / thin-provisioned storage). When the device
> +advertise that discard also zeroes data (any subsequent read after the discard
> +and before write returns zero), then mark all not-yet-zeroed inode table as
> +zeroed. This significantly speeds up filesystem initialization. This is set
> +as default.
> +.TP
> +.BI nodiscard
> +Do not attempt to discard blocks at mkfs time.
>  .RE
>  .TP
>  .BI \-f " fragment-size"
> @@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
>  @JDEV@.BR size " or " device
>  @JDEV@options can be given for a filesystem.
>  .TP
> -.BI \-K
> -Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
> -is useful on solid state devices and sparse / thin-provisioned storage).
> -.TP
>  .BI \-l " filename"
>  Read the bad blocks list from
>  .IR filename .
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 0980045..32e3a2b 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
>  				lazy_itable_init = strtoul(arg, &p, 0);
>  			else
>  				lazy_itable_init = 1;
> +		} else if (!strcmp(token, "discard")) {
> +			discard = 1;
> +		} else if (!strcmp(token, "nodiscard")) {
> +			discard = 0;
>  		} else {
>  			r_usage++;
>  			badopt = token;
> @@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
>  			"\tstripe-width=<RAID stride * data disks in blocks>\n"
>  			"\tresize=<resize maximum size in blocks>\n"
>  			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
> -			"\ttest_fs\n\n"),
> +			"\ttest_fs\n"
> +			"\tdiscard\n"
> +			"\tnodiscard\n\n"),
>  			badopt ? badopt : "");
>  		free(buf);
>  		exit(1);
> @@ -1168,7 +1174,7 @@ static void PRS(int argc, char *argv[])
>  	}
>  
>  	while ((c = getopt (argc, argv,
> -		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:KL:M:N:O:R:ST:U:V")) != EOF) {
> +		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:L:M:N:O:R:ST:U:V")) != EOF) {
>  		switch (c) {
>  		case 'b':
>  			blocksize = strtol(optarg, &tmp, 0);
> @@ -1247,9 +1253,6 @@ static void PRS(int argc, char *argv[])
>  		case 'J':
>  			parse_journal_opts(optarg);
>  			break;
> -		case 'K':
> -			discard = 0;
> -			break;
>  		case 'j':
>  			if (!journal_size)
>  				journal_size = -1;

[PATCH 5/7] mke2fs: Deprecate -K option, introduce discard/nodiscard

[Lukas Czerner]

It would be nice to have consistent "discard" options in every system
tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
and "nodiscard" is more descriptive instead of just "-K" and can be
easily defaulted and it is something we can not do with "-K".

With this commit you need to specify extended option like this:

./mke2fs -T <fstype> -E nodiscard <device>

in order make a filesystem without discarding the device first. And

./mke2fs -T <fstype> -E discard <device>

respectively.

-K option is with this commit deprecated and should not be used anymore.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.8.in |   18 +++++++++++-------
 misc/mke2fs.c    |   12 +++++++++++-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
index b46e7e2..6b437bc 100644
--- a/misc/mke2fs.8.in
+++ b/misc/mke2fs.8.in
@@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
 .I journal-options
 ]
 [
-.B \-K
-]
-[
 .B \-N
 .I number-of-inodes
 ]
@@ -240,6 +237,17 @@ enable lazy inode table initialization.
 .B test_fs
 Set a flag in the filesystem superblock indicating that it may be
 mounted using experimental kernel code, such as the ext4dev filesystem.
+.TP
+.BI discard
+Attempt to discard blocks at mkfs time (discarding blocks initially is useful
+on solid state devices and sparse / thin-provisioned storage). When the device
+advertise that discard also zeroes data (any subsequent read after the discard
+and before write returns zero), then mark all not-yet-zeroed inode table as
+zeroed. This significantly speeds up filesystem initialization. This is set
+as default.
+.TP
+.BI nodiscard
+Do not attempt to discard blocks at mkfs time.
 .RE
 .TP
 .BI \-f " fragment-size"
@@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
 @JDEV@.BR size " or " device
 @JDEV@options can be given for a filesystem.
 .TP
-.BI \-K
-Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
-is useful on solid state devices and sparse / thin-provisioned storage).
-.TP
 .BI \-l " filename"
 Read the bad blocks list from
 .IR filename .
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 0980045..5423a52 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
 				lazy_itable_init = strtoul(arg, &p, 0);
 			else
 				lazy_itable_init = 1;
+		} else if (!strcmp(token, "discard")) {
+			discard = 1;
+		} else if (!strcmp(token, "nodiscard")) {
+			discard = 0;
 		} else {
 			r_usage++;
 			badopt = token;
@@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
 			"\tstripe-width=<RAID stride * data disks in blocks>\n"
 			"\tresize=<resize maximum size in blocks>\n"
 			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
-			"\ttest_fs\n\n"),
+			"\ttest_fs\n"
+			"\tdiscard\n"
+			"\tnodiscard\n\n"),
 			badopt ? badopt : "");
 		free(buf);
 		exit(1);
@@ -1248,6 +1254,10 @@ static void PRS(int argc, char *argv[])
 			parse_journal_opts(optarg);
 			break;
 		case 'K':
+			fprintf(stderr, _("Warning: -K option is deprecated and "
+					  "should not be used anymore. Use "
+					  "\'-E nodiscard\' extended option "
+					  "instead!\n"));
 			discard = 0;
 			break;
 		case 'j':
-- 
1.7.2.3

Re: [PATCH 5/7] mke2fs: Change -K option to discard/nodiscard

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> It would be nice to have consistent "discard" options in every system
> tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
> and "nodiscard" is more descriptive instead of just "-K" and can be
> easily defaulted and it is something we can not do with "-K".
> 
> With this commit you need to specify extended option like this:
> 
> ./mke2fs -T <fstype> -E nodiscard <device>
> 
> in order make a filesystem without discarding the device first. And
> 
> ./mke2fs -T <fstype> -E discard <device>
> 
> respectively.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  misc/mke2fs.8.in |   18 +++++++++++-------
>  misc/mke2fs.c    |   13 ++++++++-----
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
> index b46e7e2..6b437bc 100644
> --- a/misc/mke2fs.8.in
> +++ b/misc/mke2fs.8.in
> @@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
>  .I journal-options
>  ]
>  [
> -.B \-K
> -]
> -[
>  .B \-N
>  .I number-of-inodes
>  ]
> @@ -240,6 +237,17 @@ enable lazy inode table initialization.
>  .B test_fs
>  Set a flag in the filesystem superblock indicating that it may be
>  mounted using experimental kernel code, such as the ext4dev filesystem.
> +.TP
> +.BI discard
> +Attempt to discard blocks at mkfs time (discarding blocks initially is useful
> +on solid state devices and sparse / thin-provisioned storage). When the device
> +advertise that discard also zeroes data (any subsequent read after the discard

advertises

> +and before write returns zero), then mark all not-yet-zeroed inode table as

inode tables

> +zeroed. This significantly speeds up filesystem initialization. This is set
> +as default.
> +.TP
> +.BI nodiscard
> +Do not attempt to discard blocks at mkfs time.

"this is the default" perhaps.  (I'm not sure any other -E option has
"no"-variants, so I'm not sure this is really needed, esp. for the
default)

>  .RE
>  .TP
>  .BI \-f " fragment-size"
> @@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
>  @JDEV@.BR size " or " device
>  @JDEV@options can be given for a filesystem.
>  .TP
> -.BI \-K
> -Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
> -is useful on solid state devices and sparse / thin-provisioned storage).
> -.TP

maybe keep this and mark as deprecated?  Not sure how Ted handles deprecated
options.  This one didn't live long ;)

>  .BI \-l " filename"
>  Read the bad blocks list from
>  .IR filename .
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 0980045..32e3a2b 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
>  				lazy_itable_init = strtoul(arg, &p, 0);
>  			else
>  				lazy_itable_init = 1;
> +		} else if (!strcmp(token, "discard")) {
> +			discard = 1;
> +		} else if (!strcmp(token, "nodiscard")) {
> +			discard = 0;
>  		} else {
>  			r_usage++;
>  			badopt = token;
> @@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
>  			"\tstripe-width=<RAID stride * data disks in blocks>\n"
>  			"\tresize=<resize maximum size in blocks>\n"
>  			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
> -			"\ttest_fs\n\n"),
> +			"\ttest_fs\n"
> +			"\tdiscard\n"
> +			"\tnodiscard\n\n"),
>  			badopt ? badopt : "");
>  		free(buf);
>  		exit(1);
> @@ -1168,7 +1174,7 @@ static void PRS(int argc, char *argv[])
>  	}
>  
>  	while ((c = getopt (argc, argv,
> -		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:KL:M:N:O:R:ST:U:V")) != EOF) {
> +		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:L:M:N:O:R:ST:U:V")) != EOF) {

again, up to Ted whether we can drop an option like this.

>  		switch (c) {
>  		case 'b':
>  			blocksize = strtol(optarg, &tmp, 0);
> @@ -1247,9 +1253,6 @@ static void PRS(int argc, char *argv[])
>  		case 'J':
>  			parse_journal_opts(optarg);
>  			break;
> -		case 'K':
> -			discard = 0;
> -			break;
>  		case 'j':
>  			if (!journal_size)
>  				journal_size = -1;

Re: [PATCH 5/7] mke2fs: Change -K option to discard/nodiscard

[Lukas Czerner]

On Tue, 16 Nov 2010, Eric Sandeen wrote:

> On 10/26/10 12:54 PM, Lukas Czerner wrote:
> > It would be nice to have consistent "discard" options in every system
> > tool (mount, fsck, mkfs) taking advantage of discards. Also "discard"
> > and "nodiscard" is more descriptive instead of just "-K" and can be
> > easily defaulted and it is something we can not do with "-K".
> > 
> > With this commit you need to specify extended option like this:
> > 
> > ./mke2fs -T <fstype> -E nodiscard <device>
> > 
> > in order make a filesystem without discarding the device first. And
> > 
> > ./mke2fs -T <fstype> -E discard <device>
> > 
> > respectively.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  misc/mke2fs.8.in |   18 +++++++++++-------
> >  misc/mke2fs.c    |   13 ++++++++-----
> >  2 files changed, 19 insertions(+), 12 deletions(-)
> > 
> > diff --git a/misc/mke2fs.8.in b/misc/mke2fs.8.in
> > index b46e7e2..6b437bc 100644
> > --- a/misc/mke2fs.8.in
> > +++ b/misc/mke2fs.8.in
> > @@ -45,9 +45,6 @@ mke2fs \- create an ext2/ext3/ext4 filesystem
> >  .I journal-options
> >  ]
> >  [
> > -.B \-K
> > -]
> > -[
> >  .B \-N
> >  .I number-of-inodes
> >  ]
> > @@ -240,6 +237,17 @@ enable lazy inode table initialization.
> >  .B test_fs
> >  Set a flag in the filesystem superblock indicating that it may be
> >  mounted using experimental kernel code, such as the ext4dev filesystem.
> > +.TP
> > +.BI discard
> > +Attempt to discard blocks at mkfs time (discarding blocks initially is useful
> > +on solid state devices and sparse / thin-provisioned storage). When the device
> > +advertise that discard also zeroes data (any subsequent read after the discard
> 
> advertises
> 
> > +and before write returns zero), then mark all not-yet-zeroed inode table as
> 
> inode tables
> 
> > +zeroed. This significantly speeds up filesystem initialization. This is set
> > +as default.
> > +.TP
> > +.BI nodiscard
> > +Do not attempt to discard blocks at mkfs time.
> 
> "this is the default" perhaps.  (I'm not sure any other -E option has
> "no"-variants, so I'm not sure this is really needed, esp. for the
> default)
> 
> >  .RE
> >  .TP
> >  .BI \-f " fragment-size"
> > @@ -369,10 +377,6 @@ and may be no more than 102,400 filesystem blocks.
> >  @JDEV@.BR size " or " device
> >  @JDEV@options can be given for a filesystem.
> >  .TP
> > -.BI \-K
> > -Keep, do not attempt to discard blocks at mkfs time (discarding blocks initially
> > -is useful on solid state devices and sparse / thin-provisioned storage).
> > -.TP
> 
> maybe keep this and mark as deprecated?  Not sure how Ted handles deprecated
> options.  This one didn't live long ;)
> 
> >  .BI \-l " filename"
> >  Read the bad blocks list from
> >  .IR filename .
> > diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> > index 0980045..32e3a2b 100644
> > --- a/misc/mke2fs.c
> > +++ b/misc/mke2fs.c
> > @@ -753,6 +753,10 @@ static void parse_extended_opts(struct ext2_super_block *param,
> >  				lazy_itable_init = strtoul(arg, &p, 0);
> >  			else
> >  				lazy_itable_init = 1;
> > +		} else if (!strcmp(token, "discard")) {
> > +			discard = 1;
> > +		} else if (!strcmp(token, "nodiscard")) {
> > +			discard = 0;
> >  		} else {
> >  			r_usage++;
> >  			badopt = token;
> > @@ -768,7 +772,9 @@ static void parse_extended_opts(struct ext2_super_block *param,
> >  			"\tstripe-width=<RAID stride * data disks in blocks>\n"
> >  			"\tresize=<resize maximum size in blocks>\n"
> >  			"\tlazy_itable_init=<0 to disable, 1 to enable>\n"
> > -			"\ttest_fs\n\n"),
> > +			"\ttest_fs\n"
> > +			"\tdiscard\n"
> > +			"\tnodiscard\n\n"),
> >  			badopt ? badopt : "");
> >  		free(buf);
> >  		exit(1);
> > @@ -1168,7 +1174,7 @@ static void PRS(int argc, char *argv[])
> >  	}
> >  
> >  	while ((c = getopt (argc, argv,
> > -		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:KL:M:N:O:R:ST:U:V")) != EOF) {
> > +		    "b:cf:g:G:i:jl:m:no:qr:s:t:vE:FI:J:L:M:N:O:R:ST:U:V")) != EOF) {
> 
> again, up to Ted whether we can drop an option like this.

This is a old version, I did send the new one "mke2fs: Deprecate -K
option, introduce discard/nodiscard" a while ago. Which removes -K
option from the man pages, and print out warning when it is used.

Thanks!
-Lukas

> 
> >  		switch (c) {
> >  		case 'b':
> >  			blocksize = strtol(optarg, &tmp, 0);
> > @@ -1247,9 +1253,6 @@ static void PRS(int argc, char *argv[])
> >  		case 'J':
> >  			parse_journal_opts(optarg);
> >  			break;
> > -		case 'K':
> > -			discard = 0;
> > -			break;
> >  		case 'j':
> >  			if (!journal_size)
> >  				journal_size = -1;
> 
> 

[PATCH 6/7] mke2fs: Use unix_discard() for discards

[Lukas Czerner]

There is generic discard function in struct_io_manager, or in
unix_io_manager to be specific. So use this instead of
mke2fs_discard_blocks().

Since mke2fs_discard_blocks() is not used anymore (and should not be)
remove it.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.c |   50 +++++++++++---------------------------------------
 1 files changed, 11 insertions(+), 39 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 32e3a2b..09ce711 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1902,48 +1902,10 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 
 #ifdef __linux__
 
-#ifndef BLKDISCARD
-#define BLKDISCARD	_IO(0x12,119)
-#endif
-
 #ifndef BLKDISCARDZEROES
 #define BLKDISCARDZEROES _IO(0x12,124)
 #endif
 
-/*
- * Return zero if the discard succeeds, and -1 if the discard fails.
- */
-static int mke2fs_discard_blocks(ext2_filsys fs)
-{
-	int fd;
-	int ret;
-	int blocksize;
-	__u64 blocks;
-	__uint64_t range[2];
-
-	blocks = ext2fs_blocks_count(fs->super);
-	blocksize = EXT2_BLOCK_SIZE(fs->super);
-	range[0] = 0;
-	range[1] = blocks * blocksize;
-
-	fd = open64(fs->device_name, O_RDWR);
-
-	if (fd > 0) {
-		ret = ioctl(fd, BLKDISCARD, &range);
-		if (verbose) {
-			printf(_("Calling BLKDISCARD from %llu to %llu "),
-			       (unsigned long long) range[0],
-			       (unsigned long long) range[1]);
-			if (ret)
-				printf(_("failed.\n"));
-			else
-				printf(_("succeeded.\n"));
-		}
-		close(fd);
-	}
-	return ret;
-}
-
 static int mke2fs_discard_zeroes_data(ext2_filsys fs)
 {
 	int fd;
@@ -2023,7 +1985,17 @@ int main (int argc, char *argv[])
 
 	/* Can't undo discard ... */
 	if (discard && (io_ptr != undo_io_manager)) {
-		retval = mke2fs_discard_blocks(fs);
+		blk64_t blocks = ext2fs_blocks_count(fs->super);
+		retval = io_ptr->discard(fs->io, 0, blocks, fs);
+
+		if (verbose) {
+			printf(_("Calling BLKDISCARD from 0 to %llu "),
+			       (unsigned long long) blocks);
+			if (retval)
+				printf(_("failed.\n"));
+			else
+				printf(_("succeeded.\n"));
+		}
 
 		if (!retval && mke2fs_discard_zeroes_data(fs)) {
 			if (verbose)
-- 
1.7.2.3

Re: [PATCH 6/7] mke2fs: Use unix_discard() for discards

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> There is generic discard function in struct_io_manager, or in
> unix_io_manager to be specific. So use this instead of
> mke2fs_discard_blocks().
> 
> Since mke2fs_discard_blocks() is not used anymore (and should not be)
> remove it.

looks fine to me, though needs fixup for the ->discard args
if that changes.

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  misc/mke2fs.c |   50 +++++++++++---------------------------------------
>  1 files changed, 11 insertions(+), 39 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 32e3a2b..09ce711 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1902,48 +1902,10 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  
>  #ifdef __linux__
>  
> -#ifndef BLKDISCARD
> -#define BLKDISCARD	_IO(0x12,119)
> -#endif
> -
>  #ifndef BLKDISCARDZEROES
>  #define BLKDISCARDZEROES _IO(0x12,124)
>  #endif
>  
> -/*
> - * Return zero if the discard succeeds, and -1 if the discard fails.
> - */
> -static int mke2fs_discard_blocks(ext2_filsys fs)
> -{
> -	int fd;
> -	int ret;
> -	int blocksize;
> -	__u64 blocks;
> -	__uint64_t range[2];
> -
> -	blocks = ext2fs_blocks_count(fs->super);
> -	blocksize = EXT2_BLOCK_SIZE(fs->super);
> -	range[0] = 0;
> -	range[1] = blocks * blocksize;
> -
> -	fd = open64(fs->device_name, O_RDWR);
> -
> -	if (fd > 0) {
> -		ret = ioctl(fd, BLKDISCARD, &range);
> -		if (verbose) {
> -			printf(_("Calling BLKDISCARD from %llu to %llu "),
> -			       (unsigned long long) range[0],
> -			       (unsigned long long) range[1]);
> -			if (ret)
> -				printf(_("failed.\n"));
> -			else
> -				printf(_("succeeded.\n"));
> -		}
> -		close(fd);
> -	}
> -	return ret;
> -}
> -
>  static int mke2fs_discard_zeroes_data(ext2_filsys fs)
>  {
>  	int fd;
> @@ -2023,7 +1985,17 @@ int main (int argc, char *argv[])
>  
>  	/* Can't undo discard ... */
>  	if (discard && (io_ptr != undo_io_manager)) {
> -		retval = mke2fs_discard_blocks(fs);
> +		blk64_t blocks = ext2fs_blocks_count(fs->super);
> +		retval = io_ptr->discard(fs->io, 0, blocks, fs);
> +
> +		if (verbose) {
> +			printf(_("Calling BLKDISCARD from 0 to %llu "),
> +			       (unsigned long long) blocks);
> +			if (retval)
> +				printf(_("failed.\n"));
> +			else
> +				printf(_("succeeded.\n"));
> +		}
>  
>  		if (!retval && mke2fs_discard_zeroes_data(fs)) {
>  			if (verbose)

[PATCH 7/7] mke2fs: Use io_manager discard_zeroes_data property

[Lukas Czerner]

We already have discard_zeroes_data variable in struct_io_manager
structure, so we do nod need mke2fs_discard_zeroes_data() function
anymore.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 misc/mke2fs.c |   27 +--------------------------
 1 files changed, 1 insertions(+), 26 deletions(-)

diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index 09ce711..114987c 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -1900,31 +1900,6 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
 	return retval;
 }
 
-#ifdef __linux__
-
-#ifndef BLKDISCARDZEROES
-#define BLKDISCARDZEROES _IO(0x12,124)
-#endif
-
-static int mke2fs_discard_zeroes_data(ext2_filsys fs)
-{
-	int fd;
-	int ret;
-	int discard_zeroes_data = 0;
-
-	fd = open64(fs->device_name, O_RDWR);
-
-	if (fd > 0) {
-		ioctl(fd, BLKDISCARDZEROES, &discard_zeroes_data);
-		close(fd);
-	}
-	return discard_zeroes_data;
-}
-#else
-#define mke2fs_discard_blocks(fs)	1
-#define mke2fs_discard_zeroes_data(fs)	0
-#endif

Re: [PATCH 7/7] mke2fs: Use io_manager discard_zeroes_data property

[Eric Sandeen]

On 10/26/10 12:54 PM, Lukas Czerner wrote:
> We already have discard_zeroes_data variable in struct_io_manager
> structure, so we do nod need mke2fs_discard_zeroes_data() function
> anymore.

OK in spirit I think but I think you may need a new home for
discard_zeroes_data.  :)

-Eric

> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  misc/mke2fs.c |   27 +--------------------------
>  1 files changed, 1 insertions(+), 26 deletions(-)
> 
> diff --git a/misc/mke2fs.c b/misc/mke2fs.c
> index 09ce711..114987c 100644
> --- a/misc/mke2fs.c
> +++ b/misc/mke2fs.c
> @@ -1900,31 +1900,6 @@ static int mke2fs_setup_tdb(const char *name, io_manager *io_ptr)
>  	return retval;
>  }
>  
> -#ifdef __linux__
> -
> -#ifndef BLKDISCARDZEROES
> -#define BLKDISCARDZEROES _IO(0x12,124)
> -#endif
> -
> -static int mke2fs_discard_zeroes_data(ext2_filsys fs)
> -{
> -	int fd;
> -	int ret;
> -	int discard_zeroes_data = 0;
> -
> -	fd = open64(fs->device_name, O_RDWR);
> -
> -	if (fd > 0) {
> -		ioctl(fd, BLKDISCARDZEROES, &discard_zeroes_data);
> -		close(fd);
> -	}
> -	return discard_zeroes_data;
> -}
> -#else
> -#define mke2fs_discard_blocks(fs)	1
> -#define mke2fs_discard_zeroes_data(fs)	0
> -#endif
> -
>  int main (int argc, char *argv[])
>  {
>  	errcode_t	retval = 0;
> @@ -1997,7 +1972,7 @@ int main (int argc, char *argv[])
>  				printf(_("succeeded.\n"));
>  		}
>  
> -		if (!retval && mke2fs_discard_zeroes_data(fs)) {
> +		if (!retval && io_ptr->discard_zeroes_data) {
>  			if (verbose)
>  				printf(_("Discard succeeded and will return 0s "
>  					 " - skipping inode table wipe\n"));