Date:   Thu, 29 Nov 2018 19:51:07 +0300
From:   Artem Blagodarenko <artem.blagodarenko@...il.com>
To:     "Theodore Y. Ts'o" <tytso@....edu>
Cc:     linux-ext4 <linux-ext4@...r.kernel.org>, adilger.kernel@...ger.ca,
        alexey.lyashkov@...il.com
Subject: Re: [PATCH v2] ext2fs: don't read group descriptors during e2label

Hello Andreas, Darric, Ted,

Thanks for review. 

> Might want to remove this ^^^^^ commented out line too…

The answer is bellow in this letter.

> On 29 Nov 2018, at 07:10, Theodore Y. Ts'o <tytso@....edu> wrote:
> 
> On Wed, Nov 28, 2018 at 11:48:40PM +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.
> 
> Are you trying to speed up using e2label to *read* the label, or
> e2label to *write* the label?  How often do you need to write the
> label, and do you need to write the label with a dirty journal?
> 
> I really don't like this patch because it makes the
> EXT2_FLAG_JOURNAL_ONLY flag an **extremely** hazardous flag to use.
> If the application uses it wrong, it could lead to extremely serious,
> disastrous data loss.
> 
> In fact, I'm going to guess that you only care about reading the
> label, and you didn't even test using "tune2fs -L new_label
> /dev/large_file_system_with_precious_data".  Because if you did, the
> tune2fs -L would call ext2fs_mark_super_dirty(), and then
> ext2fs_close() would call ext2fs_flushfs(), which would write out all
> of the block groups, writing uninitialized trash into all of the block
> group descriptor blocks beyond the first one, and your large
> production file system would be very sad.
> 
> So, ah, NACK.  :-)

 I am care about reading and writing. It is quite common operations during cluster setup, start and operation.
 Tune2fs reads superblock, journal block, fix or return label and terminates.

The only thing I am worry  is a journaling. Journal can be replied. That can lead to flushing uninitialised data.
To solve this problem I added string Darric asked for.

> +		/* don't want metadata to be flushed at close */
> +		//open_flag &= ~EXT2_FLAG_RW;

This fixes flushing uninitialised data problem on label reading codepath, but
brakes t_replay_and_set test.

-Recovering journal.
 fsck the whole mess
+test_filesys: recovering journal

Also, I just noticed it brakes label applying.

Now I understand that patch is completely bad. I would drop it and write it other way.

> If what you care about is "print label" case, what I would suggest is
> a new interface which simply reads the superblock into a struct
> ext2_super_block and calls ext2fs_swap_super() on it.  This will speed
> up the e2label read case.
> 
> If you want to speed up the e2label write case when the file system is
> cleanly unmounted, we could have a new interface that writes out the
> superblock, and tune2fs would use it if the file system does not need
> to have a journal replay.  But if the journal does need to be
> replayed, we would need to do a full ext2fs_open2() call.

Thanks. This is good idea for new patch.

> 
> When designing library interfaces, it's always important to imagine
> how an well-meaning programmer calling them might misuse them.
> Leaving land-mines for programmers to trip over --- especially when
> you trip over them yourself in the same patch which introduces the
> interfaces --- is just not a nice thing to do.  For your amusement,
> I'll leave you with:
> 
> Rusty Rusell's writeup, "How Do I Make This Hard to Misuse":
> 
>   https://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
> 
> And the flip side, his "What If I Don't Actually Like My Users?"
> 
>   https://ozlabs.org/~rusty/index.cgi/tech/2008-04-01.html

Thanks for links. I have read it. Very useful. 
Now I know that e2label -> tune2fs link brakes "The obvious use is (probably) the correct one” rule. 
I didn’t expect that e2label is not really utility in my system (despite there are sources in tree), but link to tune2fs, then faced with long label acquiring first time.
But probably I have wrong expectations.

> 
> 						- Ted

Best regards,
Artem Blagodarenko.
	

Powered by blists - more mailing lists