linux-ext4 - RE: Re: [PATCH v2] ext4: change sequential discard handling on commit complete phase into parallel manner

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20170609064212epcms1p6678460b1844ca9b16cce0e06d8bf3b1e@epcms1p6>
Date:   Fri, 09 Jun 2017 06:42:12 +0000
From:   Daeho Jeong <daeho.jeong@...sung.com>
To:     Jan Kara <jack@...e.cz>, Daeho Jeong <daeho.jeong@...sung.com>
CC:     "jack@...e.com" <jack@...e.com>, "tytso@....edu" <tytso@....edu>,
        "hch@...radead.org" <hch@...radead.org>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: RE: Re: [PATCH v2] ext4: change sequential discard handling on
 commit complete phase into parallel manner

> Hmm, here you speak about 17s, in the above paragraph about half a minute
> so what was it? Just curious...

Oh, you're are right. 17.0s is true.
 
> Actually, I didn't give my 'Reviewed-by' tag yet... But the patch looks
> generally good, some smaller comments below.

Sorry, I don't know how to use the tag exactly.

> Use list_for_each_entry_safe() instead of explicit list_entry() call.
 
> Just find the right entry in the above loop and then use
> list_cut_position() to split the list as needed. That saves us having to
> remove and add each entry. Also you don't have to use _safe() list entry
> iteration in that case.

> efd_freed can never be true here - see my explanation at the end. So you
> can simplify the code by implementing something like:
 
> void ext4_try_merge_freed_extent(struct ext4_sb_info *sbi,
>                                  struct ext4_free_data *entry,
>                                  struct ext4_free_data *new_entry)
> {
>         if ((entry->efd_tid != new_entry->efd_tid) ||
>             (entry->efd_group != new_entry->efd_group))
>                 return;
>         if (entry->efd_start_cluster + entry->efd_count ==
>             new_entry->efd_start_cluster) {
>                 new_entry->efd_start_cluster = entry->efd_start_cluster;
>                 new_entry->efd_count += entry->efd_count;
>         } else if (new_entry->efd_start_cluster + new_entry->efd_count ==
>                    entry->efd_start_cluster) {
>                 new_entry->efd_count += entry->efd_count;
>         }
>         spin_lock(&sbi->s_md_lock);
>         list_del(&entry->efd_list);
>         spin_unlock(&sbi->s_md_lock);
>         rb_erase(entry->efd_node, &(db->bb_free_root));
>         kmem_cache_free(ext4_free_data_cachep, entry);
> }
 
> which should handle all that is needed in one place.
 
> Two comments here:
 
> a) 'bool' type in structures is discouraged in kernel as it can lead to
> strange alignment issues etc. Just use int or bitfields.
 
> b) efd_freed seems in fact unnecessary. You use it only in
> ext4_freed_data_try_del() which is called only if merge succeeded which
> means tids match and thus we currently have handle open against the
> transaction with that pid and so that transaction cannot commit... So all
> in all efd_freed can never be set in the places where you call
> ext4_freed_data_try_del().
 
> Thanks for working on this!
 
I like the modified version. It looks neat.
Thank you for your valuable comments.