[Top] [All Lists]

Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/8] xfs: Pull EFI/EFD handling out from under the AIL lock
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 2 Dec 2010 12:28:41 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20101130201734.GA16079@xxxxxxxxxxxxx>
References: <1290993152-20999-1-git-send-email-david@xxxxxxxxxxxxx> <1290993152-20999-2-git-send-email-david@xxxxxxxxxxxxx> <20101130201734.GA16079@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue, Nov 30, 2010 at 03:17:34PM -0500, Christoph Hellwig wrote:
>  - xfs_efi_init needs to initialize efi_next_extent using ATOMIC_INIT


>  - there is a behaviour change about the xfs_trans_del_item call
>    in xfs_efi_item_unpin - before it was protected by the
>    XFS_EFI_CANCELED which was never set, and now it's not.

XFS_EFI_CANCELED has not been set in the code base since
xfs_efi_cancel() was removed back in 2006 by commit
065d312e15902976d256ddaf396a7950ec0350a8 ("[XFS] Remove unused
iop_abort log item operation), and even then xfs_efi_cancel() was
never called. I haven't tracked it back further than that (beyond
git history), but handling of efis in cancelled transactions has
been broken for a long time.

Basically, when we get an IOP_UNPIN(lip, 1); call from
xfs_trans_uncommit() (i.e. remove == 1), if we don't free the log
item descriptor we leak it. IOWs, the new behaviour introduced in
this patch is actually the correct behaviour.

>  - what happened to XFS_EFI_RECOVERED?  You changed it to be indexed
>    for the atomic bit-ops, but it's still used non-atomic in the log
>    recovery code.

Ah, I forgot to convert it.

>  - Why is XFS_EFI_COMMITTED cleared in xlog_recover_do_efi_trans,
>    where it can't ever be set?

It was just a straight transformation. I'll kill it.

>  - can you please add a shared helper for xfs_efi_item_unpin and
>    xfs_efi_release, ala:

Ok. Will do.


Dave Chinner

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