xfs
[Top] [All Lists]

Re: [PATCH] xfs: add discard support (at transaction commit)

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: add discard support (at transaction commit)
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Mon, 10 May 2010 07:59:44 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100509175048.GA1435@xxxxxxxxxxxxx>
References: <20100509175048.GA1435@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Sun, May 09, 2010 at 01:50:48PM -0400, Christoph Hellwig wrote:
> Now that we have reliably tracking of deleted extents in a transaction
> we can easily implement "online" discard support which calls
> blkdev_issue_discard once a transaction commits.  We simply have to
> walk the list of busy extents after transaction commit, but before deleting
> it from the rbtree tracking these busy extents.
> 
> This does not replace by background discard support patch which is probably
> better for thin provisioned arrays - I will updated it to apply ontop of
> this patch when I'm ready to re-submit it.

I think this can  be made to work, but I don't really like it that
much, especially the barrier flush part. Is there any particular
reason we need to issue discards at this level apart from "other
filesystems are doing it" rather than doing it lazily in a
non-performance critical piece of code?

Regardless of this, some questions about the patch come to mind:

        1. is it safe to block the xfslogd in the block layer in,
        say, get_request()? i.e. should we be issuing IO from an IO
        completion handler? That raises red flags in my head...

        2. issuing discards will block xfslogd and potentially stall
        the log if there are lots of discards to issue.

        3. DISCARD_FL_BARRIER appears to be used to allow async
        issuing of the discard to ensure any followup write has the
        discard processed first. What happens if the device does not
        support barriers or barriers are turned off?

        To me it appears that a lack of barriers could result in a
        write being reordered in front of the discard.  e.g.
        delalloc results in btree block freed, marked busy. New
        delalloc occurs, allocates block, marked sync, forces log,
        issues async discard, completes transaction and then writes
        data async.  Which operation does the drive see and complete
        first - the discard or the data write?

        4. A barrier IO on every discard? In a word: Ouch.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>