From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay1.corp.sgi.com [137.38.102.111]) by oss.sgi.com (8.14.3/8.14.3/SuSE Linux 0.8) with ESMTP id p4KDeKdW093995 for ; Fri, 20 May 2011 08:40:20 -0500 Subject: Re: [PATCH 1/4] xfs: add online discard support From: Alex Elder In-Reply-To: <20110520102430.GA18199@infradead.org> References: <20110504185513.136746538@bombadil.infradead.org> <20110504190011.156999943@bombadil.infradead.org> <1305842024.2825.86.camel@doink> <20110520102430.GA18199@infradead.org> Date: Fri, 20 May 2011 08:40:16 -0500 Message-ID: <1305898816.3976.19.camel@doink> MIME-Version: 1.0 Reply-To: aelder@sgi.com List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: xfs-bounces@oss.sgi.com Errors-To: xfs-bounces@oss.sgi.com To: Christoph Hellwig Cc: xfs@oss.sgi.com On Fri, 2011-05-20 at 06:24 -0400, Christoph Hellwig wrote: > On Thu, May 19, 2011 at 04:53:44PM -0500, Alex Elder wrote: > > The first is, why not support it for non-delaylog? > > Because: > > a) performance is going to suck even more horribly with the > amount of trim commands needed, with no chance of actually > fixing it > b) the async discard code in patch 3 not easily applyable to > the non-delaylog case, we'd need to keep two parallel codebases, > one of them guaranteed to be untested. I hadn't looked through all of the patches yet. > > Second, why is it a two phase operation (marking an > > extent for discard, then doing all the discards at > > once)? Is it just so you can do the discards without > > holding the perag lock? > > Because we must prevent the allocation code from reusing an extent > that is undergoing a discard right now to prevent corruption, thus > we need to mark it as do not touch first. OK. > > > xfs_trans_committed_bulk(ctx->cil->xc_log->l_ailp, ctx->lv_chain, > > > ctx->start_lsn, abort); > > > > > > xfs_alloc_busy_sort(&ctx->busy_extents); > > > > I still think sorting the list belongs inside xfs_alloc_busy_clear(). > > I see that list_sort() is not necessarily trivial for an already > > sorted list though... > > It's a bad idea to do the sort twice for no good reason, and adding > another parameter to further overload xfs_alloc_busy_clear behaviour > doesn't seem smart either. It's fine, it's just the purist in me that wants the interface to hide the dirty work of sorting. But I concede your points here--not a big deal. > > if (error == EOPNOTSUPP) { > > /* > > * Report this once per mount point somehow? > > * If so, turn off the mount option? > > */ > > break; > > We've been through this discussion again lately with dm and ext4 > folks, and the conclusion is that EOPNOTSUPP is perfectly fine to happen > here. The main thing I wanted to suggest was dropping out after the first one, but considering the underlying device may be a compound logical volume I understand now why it's better to go through them all. (I hadn't been paying attention to the dm and ext4 discussion.) > > > + * performing the discard a chance to mark the extent unbusy > > > + * and retry. > > > + */ > > > + if (busyp->flags & XFS_ALLOC_BUSY_DISCARDED) { > > > + spin_unlock(&pag->pagb_lock); > > > + delay(1); > > > > I hate seeing calls to delay() although sometimes > > it's the right thing to do... I don't have a feel > > for how long a discard is likely to take so I don't > > know whether waiting here instead would be worth > > the effort. > > It's not nice, but if the block layer gets fixed and we do asynchronous > discards it simply goes away. Sounds good. > > If this option is to only be available for delaylog, it should > > say so here (and maybe report that it's being ignored if it's > > supplied with nodelaylog at mount time). > > ok. > Thanks for answering my questions. I'll look for an update but unless there's something obviously wrong I'll turn it around pretty quickly so this gets in during the merge window. -Alex _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs