Re: [PATCH] aio: using hash table for active requests

[Jeff Moyer <jmoyer@redhat.com>]

Li Yu <raise.sail@gmail.com> writes:

> This patch remove a TODO in fs/aio.c, that is to use hash table for
> active requests.

It's remained a TODO item for this long because it hasn't really
mattered in the past.  Was there other motivation for this patch besides
the TODO comment?

> I prefer add an iocb at tail of collision chain, so I do not use hlist
> here.

Why do you prefer to add to the tail?  It makes more sense to add
collisions at the head, since if you're going to cancel everything, you
have a better chance of cancelling stuff that was queued later than
earlier (assuming you can cancel anything at all, which for most cases,
you can't).  Also, you're halving the number of buckets, so you should
have a good reason.

What sort of testing did you do?

I've made some cursory comments below.  I'll more fully review this if
you can provide a bit more justification and some proof of testing.  I'd
be really surprised if this helped any real workloads, though.

Cheers,
Jeff

> +static int ioctx_active_reqs_init(struct kioctx *ctx)
> +{
> +	int i;
> +
> +	ctx->active_reqs_table = kmalloc(AIO_ACTREQ_BUCKETS*sizeof(struct list_head), GFP_KERNEL);

Fit this into 80 columns, please.  You may just want to run checkpatch
over the whole thing.

> +static inline void aio_cancel_one(struct kioctx *ctx, struct kiocb *iocb)

I see no good reason to inline this.

> @@ -465,10 +506,12 @@ static struct kiocb *__aio_get_req(struct kioctx *ctx)
>  	/* Check if the completion queue has enough free space to
>  	 * accept an event from this io.
>  	 */
> +	bucket = hash_long((unsigned long)tohash, AIO_ACTREQ_BUCKETS_SHIFT);

hash_ptr?

> -	struct list_head	active_reqs;	/* used for cancellation */
> +	struct list_head*	active_reqs_table;	/* used for cancellation */

Coding Style