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.
|