xfs
[Top] [All Lists]

Re: [PATCH] xfs: fix buffer flushing during unmount

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: fix buffer flushing during unmount
From: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
Date: Wed, 12 Oct 2011 10:15:49 +0530
Cc: xfs@xxxxxxxxxxx
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; bh=XAhFPoWGU1etKA4N8RIHdDGEgizT+n/1JwWhX3OCxZo=; b=KhdBXO0qkoAh2MP0bR8ptR+7TG+6hWAiyNHbNNffn5y93RgnLtxu6//bHACVEtiO0m XAQb9hnm1TEYoqKrBFuItuY0jfJqMQYH9VfLzm4Xa31SgdEy892YPyKwwGr5qZCxTo0Z CJLNnwk8ycgHtKNTjw6BC7whRN25BUHV3wt0k=
In-reply-to: <20110914140826.GA25729@xxxxxxxxxxxxx>
References: <20110914140826.GA25729@xxxxxxxxxxxxx>
Hi Christoph,
Just few minor suggestions to maintain modularity:

On Wed, Sep 14, 2011 at 7:38 PM, Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote:
> The code to flush buffers in the umount code is a bit iffy: we first flush
> all delwri buffers out, but then might be able to queue up a new one when
> logging the sb counts.  On a normal shutdown that one would get flushed
> out when doing the synchronous superblock write in xfs_unmountfs_writesb,
> but we skip that one if the filesystem has been shut down.
>
> Fix this by moving the delwri list flushing until just before unmounting
> the log, and while we're at it also remove the superflous delwri list
> and buffer lru flusing for the rt and log device that can never have
> cached or delwri buffers.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reported-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
> Tested-by: Amit Sahrawat <amit.sahrawat83@xxxxxxxxx>
>
> Index: xfs/fs/xfs/xfs_buf.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_buf.h   2011-09-13 10:55:20.744090516 -0400
> +++ xfs/fs/xfs/xfs_buf.h        2011-09-13 10:55:52.461091480 -0400
> @@ -318,7 +318,6 @@ extern struct list_head *xfs_get_buftarg
>  #define xfs_getsize_buftarg(buftarg)   block_size((buftarg)->bt_bdev)
>  #define xfs_readonly_buftarg(buftarg)  bdev_read_only((buftarg)->bt_bdev)
>
> -#define xfs_binval(buftarg)            xfs_flush_buftarg(buftarg, 1)
>  #define XFS_bflush(buftarg)            xfs_flush_buftarg(buftarg, 1)
>
>  #endif /* __XFS_BUF_H__ */
> Index: xfs/fs/xfs/xfs_mount.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_mount.c 2011-09-13 10:55:20.748087866 -0400
> +++ xfs/fs/xfs/xfs_mount.c      2011-09-13 10:56:19.108088343 -0400
> @@ -44,9 +44,6 @@
>  #include "xfs_trace.h"
>
>
> -STATIC void    xfs_unmountfs_wait(xfs_mount_t *);
> -
> -
>  #ifdef HAVE_PERCPU_SB
>  STATIC void    xfs_icsb_balance_counter(xfs_mount_t *, xfs_sb_field_t,
>                                                int);
> @@ -1496,11 +1493,6 @@ xfs_unmountfs(
>         */
>        xfs_log_force(mp, XFS_LOG_SYNC);
>
> -       xfs_binval(mp->m_ddev_targp);
> -       if (mp->m_rtdev_targp) {
> -               xfs_binval(mp->m_rtdev_targp);
> -       }
> -
This is OK.
>        /*
>         * Unreserve any blocks we have so that when we unmount we don't 
> account
>         * the reserved free space as used. This is really only necessary for
> @@ -1526,7 +1518,16 @@ xfs_unmountfs(
>                xfs_warn(mp, "Unable to update superblock counters. "
>                                "Freespace may not be correct on next mount.");
>        xfs_unmountfs_writesb(mp);
> -       xfs_unmountfs_wait(mp);                 /* wait for async bufs */
> +
> +       /*
> +        * Make sure all buffers have been flushed and completed before
> +        * unmounting the log.
> +        */
> +       error = xfs_flush_buftarg(mp->m_ddev_targp, 1);
> +       if (error)
> +               xfs_warn(mp, "%d busy buffers during unmount.", error);
> +       xfs_wait_buftarg(mp->m_ddev_targp);
> +
instead of removing xfs_unmountfs_wait() altogether, how about keeping
the function and modifying the contents with above changes?
flushing/waiting at this point looks a little out of order.
>        xfs_log_unmount_write(mp);
>        xfs_log_unmount(mp);
>        xfs_uuid_unmount(mp);
> @@ -1537,16 +1538,6 @@ xfs_unmountfs(
>        xfs_free_perag(mp);
>  }
>
> -STATIC void
> -xfs_unmountfs_wait(xfs_mount_t *mp)
> -{
> -       if (mp->m_logdev_targp != mp->m_ddev_targp)
> -               xfs_wait_buftarg(mp->m_logdev_targp);
> -       if (mp->m_rtdev_targp)
> -               xfs_wait_buftarg(mp->m_rtdev_targp);
> -       xfs_wait_buftarg(mp->m_ddev_targp);
> -}
> -
I mean the above should be like this:
STATIC void
xfs_unmountfs_wait(xfs_mount_t *mp)
{
 +
 +       /*
 +        * Make sure all buffers have been flushed and completed before
 +        * unmounting the log.
 +        */
 +       error = xfs_flush_buftarg(mp->m_ddev_targp, 1);
 +       if (error)
 +               xfs_warn(mp, "%d busy buffers during unmount.", error);
 +       xfs_wait_buftarg(mp->m_ddev_targp);
 +
}

>  int
>  xfs_fs_writable(xfs_mount_t *mp)
>  {
>
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs
>

Regards,
Amit Sahrawat

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