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@xxxxxxx> 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.
