From: Jan Kara Subject: Re: RE: Re: [PATCH] ext4: change sequential discard handling on commit complete phase into parallel manner Date: Wed, 31 May 2017 12:12:40 +0200 Message-ID: <20170531101240.GC14011@quack2.suse.cz> References: <20170531004926epcms1p5ea300c8e634f7a0a42af283c4df9a1b7@epcms1p5> <20170530091626.GA3284@quack2.suse.cz> <1496113566-18899-1-git-send-email-daeho.jeong@samsung.com> <20170531022240epcms1p27bf64e351cc61e84378ea8de38e192ac@epcms1p2> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Jan Kara , "jack@suse.com" , "hch@infradead.org" , "tytso@mit.edu" , "linux-ext4@vger.kernel.org" To: Daeho Jeong Return-path: Received: from mx2.suse.de ([195.135.220.15]:40271 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751096AbdEaKMx (ORCPT ); Wed, 31 May 2017 06:12:53 -0400 Content-Disposition: inline In-Reply-To: <20170531022240epcms1p27bf64e351cc61e84378ea8de38e192ac@epcms1p2> Sender: linux-ext4-owner@vger.kernel.org List-ID: 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 SUSE Labs, CR