xfs
[Top] [All Lists]

Re: [PATCH] xfs: fix the xfs_iflush_done callback search

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH] xfs: fix the xfs_iflush_done callback search
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 2 Oct 2014 08:34:56 +1000
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20141001191803.153063401@xxxxxxx>
References: <20141001191803.153063401@xxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Wed, Oct 01, 2014 at 04:18:02PM -0500, Mark Tinguely wrote:
> Commit "xfs: remove all the inodes on a buffer from the AIL in bulk"
> made the xfs inode flush callback more efficient by combining all
> the inode writes on the buffer and the deletions of the inode log
> item from AIL.
> 
> The initial loop in this patch should be looping through all
> the log items on the buffer to see which items have
> xfs_iflush_done as their callback function. But currently,
> only the log item passed to the function has its callback
> compared to xfs_iflush_done. If the log item pointer passed to
> the function does have the xfs_iflush_done callback function,
> then all the log items on the buffer are removed from the
> li_bio_list on the buffer b_fspriv and could be removed from
> the AIL eventhough they may have not been written yet.

Looks like a bug, but what I don't know from this description is
the symptoms and impact of this bug being hit? Is there a risk of
filesystem corruption on crash or power loss? Perhaps it's a data
loss issue? I can't tell, and for anyone scanning the commit logs to
determine if they need to backport the fix will be asking the same
questions.

Also, is there a reproducable test case for it?

> Signed-off-by: Mark Tinguely <tinguely@xxxxxxx>
> ---
>  fs/xfs/xfs_inode_item.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/fs/xfs/xfs_inode_item.c
> ===================================================================
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -615,7 +615,7 @@ xfs_iflush_done(
>       blip = bp->b_fspriv;
>       prev = NULL;
>       while (blip != NULL) {
> -             if (lip->li_cb != xfs_iflush_done) {
> +             if (blip->li_cb != xfs_iflush_done) {

Looks correct, but more info needed...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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