linux-ext4 - Re: [PATCH] ext2fs: add ext2fs_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <EDEB5AE3-80AC-43D7-8726-4D9E45D710FE@gmail.com>
Date:   Thu, 3 Oct 2019 19:18:47 +0300
From:   Благодаренко Артём 
        <artem.blagodarenko@...il.com>
To:     "Darrick J. Wong" <darrick.wong@...cle.com>
Cc:     linux-ext4@...r.kernel.org, adilger.kernel@...ger.ca
Subject: Re: [PATCH] ext2fs: add ext2fs_read_sb that returns superblock

Darrick, thanks for feedback.

> On 3 Oct 2019, at 19:01, Darrick J. Wong <darrick.wong@...cle.com> wrote:
> 
> On Thu, Sep 05, 2019 at 02:01:10PM +0300, Artem Blagodarenko wrote:
>> tune2fs is used to make e2label duties.  ext2fs_open2() reads group
>> descriptors which are not used during disk label obtaining, but takes
>> a lot of time on large partitions.
>> 
>> This patch adds ext2fs_read_sb(), there only initialized superblock
>> is returned This saves time dramatically.
>> 
>> Signed-off-by: Artem Blagodarenko <c17828@...y.com>
>> Cray-bug-id: LUS-5777
>> ---
>> lib/ext2fs/ext2fs.h |  2 ++
>> lib/ext2fs/openfs.c | 16 ++++++++++++++++
>> misc/tune2fs.c      | 23 +++++++++++++++--------
>> 3 files changed, 33 insertions(+), 8 deletions(-)
>> 
>> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
>> index 59fd9742..3a63b74d 100644
>> --- a/lib/ext2fs/ext2fs.h
>> +++ b/lib/ext2fs/ext2fs.h
>> @@ -1630,6 +1630,8 @@ extern int ext2fs_journal_sb_start(int blocksize);
>> extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
>> 			     unsigned int block_size, io_manager manager,
>> 			     ext2_filsys *ret_fs);
>> +extern errcode_t ext2fs_read_sb(const char *name, io_manager manager,
>> +				struct ext2_super_block * super);
>> extern errcode_t ext2fs_open2(const char *name, const char *io_options,
>> 			      int flags, int superblock,
>> 			      unsigned int block_size, io_manager manager,
>> diff --git a/lib/ext2fs/openfs.c b/lib/ext2fs/openfs.c
>> index 51b54a44..95f45d84 100644
>> --- a/lib/ext2fs/openfs.c
>> +++ b/lib/ext2fs/openfs.c
>> @@ -99,6 +99,22 @@ static void block_sha_map_free_entry(void *data)
>> 	return;
>> }
>> 
>> +errcode_t ext2fs_read_sb(const char *name, io_manager manager,
>> +			 struct ext2_super_block * super)
>> +{
>> +	io_channel	io;
>> +	errcode_t	retval = 0;
>> +
>> +	retval = manager->open(name, 0, &io);
>> +	if (!retval) {
>> +		retval = io_channel_read_blk(io, 1, -SUPERBLOCK_SIZE,
>> +				     super);
>> +		io_channel_close(io);
>> +	}
>> +
>> +	return retval;
>> +}
>> +
>> /*
>>  *  Note: if superblock is non-zero, block-size must also be non-zero.
>>  * 	Superblock and block_size can be zero to use the default size.
>> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
>> index 7d2d38d7..fea607e1 100644
>> --- a/misc/tune2fs.c
>> +++ b/misc/tune2fs.c
>> @@ -2879,6 +2879,21 @@ int tune2fs_main(int argc, char **argv)
>> #endif
>> 		io_ptr = unix_io_manager;
>> 
>> +	if (print_label) {
>> +		/* For e2label emulation */
>> +		struct ext2_super_block sb;
>> +
>> +		/* Read only superblock. Nothing else metters.*/
> 
>                                                      matters. */
> 
>> +		retval = ext2fs_read_sb(device_name, io_ptr, &sb);
>> +		if (!retval) {
>> +			printf("%.*s\n", (int) sizeof(sb.s_volume_name),
>> +			       sb.s_volume_name);
>> +		}
> 
> Um, does this drop the error without making a report?

No error message to output, but error code is returned to pointer.
I believe user expect only disk label, no other output.

> 
>> +
>> +		remove_error_table(&et_ext2_error_table);
>> +		return retval;
>> +	}
> 
> I wonder if ext[24] should implement FS_IOC_[GS]ETFSLABEL for mounted
> filesystems … ?

Probably not very useful for Lustre FS, there disk label is not needed during cluster work.
Darrick, can you suggest any use case for this?
Also such features need to be added in separate patch. This patch is about performance optimisation. 

Best regards,
Artem Blagodarenko.
> 
> --D
> 
>> +
>> retry_open:
>> 	if ((open_flag & EXT2_FLAG_RW) == 0 || f_flag)
>> 		open_flag |= EXT2_FLAG_SKIP_MMP;
>> @@ -2972,14 +2987,6 @@ retry_open:
>> 	sb = fs->super;
>> 	fs->flags &= ~EXT2_FLAG_MASTER_SB_ONLY;
>> 
>> -	if (print_label) {
>> -		/* For e2label emulation */
>> -		printf("%.*s\n", (int) sizeof(sb->s_volume_name),
>> -		       sb->s_volume_name);
>> -		remove_error_table(&et_ext2_error_table);
>> -		goto closefs;
>> -	}
>> -
>> 	retval = ext2fs_check_if_mounted(device_name, &mount_flags);
>> 	if (retval) {
>> 		com_err("ext2fs_check_if_mount", retval,
>> -- 
>> 2.14.3