xfs
[Top] [All Lists]

Re: xfs: add FITRIM support

To: Alex Elder <aelder@xxxxxxx>
Subject: Re: xfs: add FITRIM support
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Tue, 28 Dec 2010 11:09:40 -0500
Cc: xfs@xxxxxxxxxxx, lczerner@xxxxxxxxxx
In-reply-to: <1293054073.2408.374.camel@doink>
References: <20101125112304.GA4195@xxxxxxxxxxxxx> <1293054073.2408.374.camel@doink>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Dec 22, 2010 at 03:41:13PM -0600, Alex Elder wrote:
> > +   error = xfs_alloc_read_agf(mp, NULL, agno,
> > +                              XFS_ALLOC_FLAG_TRYLOCK, &agbp);
> > +   if (error || !agbp) {
> > +           if (error == EAGAIN)
> > +                   error = 0;
> 
> EAGAIN is ignored because it's an advisory interface, right?
> How hard are we expected to try?  What I really mean is,
> is the benefit of FITRIM enough that we should try again
> later when we can get a buffer or lock on it?

That was the idea when I wrote this code.  But back then we called it
regularly from a kernel thread.  For FITRIM it makes more sense to just
remove the trylock.

> I don't know where (or if) FITRIM is precisely documented.
> But I question whether truncating down the start offset is
> the correct thing to do.  If the starting byte offset given
> were not block-aligned, it seems like you should not assume
> that the caller wanted the bytes below unmapped.  (This is
> a broader question, not related directly to your change.)
> 
> Similarly, on the length it is probably best to truncate
> it, because it avoids any bytes beyond the specified range
> getting unmapped.  (I.e., in my mind what you did is the
> right way to do it.)  But these interpretations are
> dependent on the specific interpretation of FITRIM...

Good question.  Adding Lukas to the Cc.  I tried to talk him into
writing a manpage to document the interface better, but that's only
been a few days before the holidays.  This is something we should
documented.  I don't quite understand the need for the range interface
anyway.

> You don't update range anywhere, so the copyout below
> is not really doing anything useful.  However I think
> it should stay, and the number of bytes actually
> trimmed should be updated and returned to the user.
> That seems to be what ext4 does (the only reference
> I found at the moment for what FITRIM is supposed
> to return).

Yes, I guess I should update the range.

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