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@xxxxxxxx>
SUSE Labs, CR
