xfs
[Top] [All Lists]

Re: [PATCH 2/5] xfs: use per-filesystem I/O completion workqueues

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 2/5] xfs: use per-filesystem I/O completion workqueues
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Thu, 17 Nov 2011 10:24:30 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111115201426.686729205@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111115201407.038216766@xxxxxxxxxxxxxxxxxxxxxx> <20111115201426.686729205@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Tue, Nov 15, 2011 at 03:14:09PM -0500, Christoph Hellwig wrote:
> commit 77d7a0c "xfs: Non-blocking inode locking in IO completion" introduced
> a trylocked and defer scheme in xfs_setfilesize to avoid deadlocks when on
> XFS filesystem is used ontop of another using the loop device, and we
> fsync in the loop filesystem.
> 
> Now that we have the cheap enough concurrency managed workqueues, we can
> create per-filesystem instead of global workqueues and remove this scheme
> again, given that it has the potential of delaying size updates and is not
> helpful once we start to log the inode size.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

....

>  
> +STATIC int
> +xfs_init_mount_workqueues(
> +     struct xfs_mount        *mp)
> +{
> +     snprintf(mp->m_data_workqueue_name, XFS_WQ_NAME_LEN,
> +              "xfs-data/%s", mp->m_fsname);
> +     mp->m_data_workqueue =
> +             alloc_workqueue(mp->m_data_workqueue_name, WQ_MEM_RECLAIM, 1);
> +     if (!mp->m_data_workqueue)
> +             goto out;
> +
> +     snprintf(mp->m_unwritten_workqueue_name, XFS_WQ_NAME_LEN,
> +              "xfs-conv/%s", mp->m_fsname);
> +     mp->m_unwritten_workqueue =
> +             alloc_workqueue(mp->m_unwritten_workqueue_name,
> +                             WQ_MEM_RECLAIM, 1);
> +     if (!mp->m_unwritten_workqueue)
> +             goto out_destroy_data_iodone_queue;
> +
> +     return 0;
> +
> +out_destroy_data_iodone_queue:
> +     destroy_workqueue(mp->m_data_workqueue);
> +out:
> +     return -ENOMEM;
> +#undef XFS_WQ_NAME_LEN

Don't need the undef here anymore as the #define is in the header
file and not local to the function.

> Index: linux-2.6/fs/xfs/xfs_mount.h
> ===================================================================
> --- linux-2.6.orig/fs/xfs/xfs_mount.h 2011-11-10 16:50:40.787796774 +0100
> +++ linux-2.6/fs/xfs/xfs_mount.h      2011-11-15 09:15:25.053316473 +0100
> @@ -211,6 +211,12 @@ typedef struct xfs_mount {
>       struct shrinker         m_inode_shrink; /* inode reclaim shrinker */
>       int64_t                 m_low_space[XFS_LOWSP_MAX];
>                                               /* low free space thresholds */
> +
> +     struct workqueue_struct *m_data_workqueue;
> +     struct workqueue_struct *m_unwritten_workqueue;
> +#define XFS_WQ_NAME_LEN              512
> +     char                    m_data_workqueue_name[XFS_WQ_NAME_LEN];
> +     char                    m_unwritten_workqueue_name[XFS_WQ_NAME_LEN];
>  } xfs_mount_t;

Can't say I'm a great fan of this - making the workqueue code use
kstrdup() would be a much better better solution, IMO, just like was
done a while for the SLAB cache names to solve exactly the same
problem....

Regardless,

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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