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