linux-ext4 - Re: RE: Re: [PATCH] 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: <20170531101240.GC14011@quack2.suse.cz>
Date:   Wed, 31 May 2017 12:12:40 +0200
From:   Jan Kara <jack@...e.cz>
To:     Daeho Jeong <daeho.jeong@...sung.com>
Cc:     Jan Kara <jack@...e.cz>, "jack@...e.com" <jack@...e.com>,
        "hch@...radead.org" <hch@...radead.org>,
        "tytso@....edu" <tytso@....edu>,
        "linux-ext4@...r.kernel.org" <linux-ext4@...r.kernel.org>
Subject: Re: RE: Re: [PATCH] ext4: change sequential discard handling on
 commit complete phase into parallel manner

On Wed 31-05-17 02:22:40, Daeho Jeong wrote:
> Hi Jan,
> 
> > Hi Jan,
>  
> > > Hum, these games with several callbacks, lists, etc. look awkward and
> > > unnecessary. It think they mostly come from the fact that we call separate
> > > freeing callback for each extent to free which doesn't fit the needs of
> > > async discard well.
>  
> > > So instead of adding post_cb_list and several callback functions, it would
> > > seem easier to have just one callback structure instead of one for every
> > > extent. Then the structure would contain a list of extents that need to be
> > > freed freed. So something like:
>  
> > > struct ext4_free_data {
> > >         struct ext4_journal_cb_entry efd_jce;
> > >         struct list_head efd_extents;
> > > }
>  
> > > struct ext4_freed_extent {
> > >         struct list_head efe_list;
> > >         struct rb_node efe_node;
> > >         ext4_group_t efe_group;
> > >         ext4_grpblk_t efe_start_cluster;
> > >         ext4_grpblk_t efe_count;
> > >         tid_t efe_tid;
> > > }
>  
> > > When commit happens, we can just walk the efd_extents list while efe_tid is
> > > equal tid of the transaction for which the callback was called and submit all
> > > discard requests. You can use bio chaining implemented in
> > > __blkdev_issue_discard() which XFS already uses and so the result of all
> > > the discards you submit will be just one bio. Then you walk the list of
> > > extents again and free them in the buddy bitmaps. And finally, you wait for
> > > the bio to complete. All will be then happening in one function and it will
> > > be much easier to understand.
>  
> > It's right. the patch didn't look neat because of a few callbacks and the
> > post callback list. I will modify the patch as your suggestion. It will
> > look better.
>  
> > Thank you very much. :-)
> 
> It's a little difficult to decide when we have to add new ext4_free_data
> entry for a transaction for the first time and how do we know whether the
> ext4_free_data entry for a transaction is already added or not? I think
> that it is a bad idea to search in t_private_list of the transaction for
> that, because there might be the different type of callback entries in
> the future.
> 
> And how do we find the exact ext4_free_data entry for a newly created
> ext4_freed_extent?  We only know which transcation is related to the
> ext4_freed_extent, so we could use this but I don't have any good idea
> for that. 

Right, so looking at ext4_journal_commit_callback() it will be probably the
easiest to just call some new function ext4_process_freed_extents(sb)
directly from there (i.e., avoid the ext4 callback infrastructure
altogether) and anchor the list of extents in the superblock.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR