Re: [PATCH 2/3] xfs: Don't issue buffer IO direct from AIL push

[Christoph Hellwig <hch@infradead.org>]

> +/*
> + * If a delwri buffer needs to be pushed before it has aged out, then
> + * promote it to the head of the delwri queue so that it will be flushed
> + * on the next xfsbufd run.
> + */
> +void
> +xfs_buf_delwri_promote(
> +	xfs_buf_t	*bp)
> +{
> +	struct list_head *dwq = &bp->b_target->bt_delwrite_queue;
> +	spinlock_t	*dwlk = &bp->b_target->bt_delwrite_lock;
> +	long		age = xfs_buf_age_centisecs * msecs_to_jiffies(10) + 1;
> +
> +	spin_lock(dwlk);
> +	ASSERT(bp->b_flags & XBF_DELWRI);
> +	ASSERT(bp->b_flags & _XBF_DELWRI_Q);
> +	list_del(&bp->b_list);
> +	list_add(&bp->b_list, dwq);
> +	bp->b_queuetime = jiffies - age;
> +	spin_unlock(dwlk);

Sorry for the nitpicking, but:

 a) can you use the struct types instead of the typedefs where possible?
 b) second the pointer to spinlock style used here like in some other
    buf code is rather odd.  What about this instead:

void
xfs_buf_delwri_promote(
	struct xfs_buf		*bp)
{
	struct xfs_buftarg	*target = bp->b_target;

	spin_lock(&target->bt_delwrite_lock);
	ASSERT(bp->b_flags & XBF_DELWRI);
	ASSERT(bp->b_flags & _XBF_DELWRI_Q);

	list_move(&bp->b_list, &target->bt_delwrite_queue);
	bp->b_queuetime = jiffies -
			  xfs_buf_age_centisecs * msecs_to_jiffies(10) - 1;
	spin_unlock(&target->bt_delwrite_lock);
}

Also the queuetime calculation could use some comments.

>  extern void xfs_wait_buftarg(xfs_buftarg_t *);
>  extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int, unsigned int);
>  extern int xfs_flush_buftarg(xfs_buftarg_t *, int);
> +
> +/*
> + * run the xfsbufd on demand to age buffers. Use in combination with
> + * xfs_buf_delwri_promote() to flus delayed write buffers efficiently.
> + */
> +static inline void xfs_flush_buftarg_delwri(xfs_buftarg_t *btp)
> +{
> +	wake_up_process(btp->bt_task);
> +}

The function name is extremly misleading.  It's an xfsbufd wakeup, so it
should be named like that.  In doubt I'd just opencode the
wake_up_process call instead.


The changes to the various log items look good, especially as we bring
some more commonality into the various items.


You removed the only call to trace_xfs_inode_item_push, so you might
aswell remove the trace point declaration, too.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs