On Thu, 01 Sep 2011 09:56:38 -0400
Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> >> ...
> >>
> >> +#define KIOCB_STASH_SIZE 32
> >> +struct kiocb_stash {
> >> + unsigned head;
> >> + unsigned tail;
> >> + unsigned total;
> >> + unsigned used;
> >> + struct kiocb *kiocbs[KIOCB_STASH_SIZE];
> >> +};
> >
> > Laziness begets laziness: you didn't bother documenting how this ring
> > thing works and why it exists, so I didn't bother reverse-engineering
> > that infomation.
>
> Sorry. I used a ring as I didn't want to introduce more allocations in
> the submission path (though I have no real data on which to base this
> premature optimization).
A list_head in the kiocb wouldn't require additional allocation
operations.
> As for documenting how it works, I thought it
> was pretty straight-forward.
Well you would, you wrote it! Others must go backwards from the
implementation and arrive at the model you had in your mind at the
time.
That isn't super-hard of course, but when I find myself scratching my
head even a small amount over what was in your mind, that's when I
stop. Because I know that everyone else who reads this code will also
end up scratching their head.
> > Once all this has been fixed, I might be wondering why it was done with
> > a yukky fixed-size array rather than with a list.
>
> I could change it to a list, if that's preferred.
To prefer that I'd need to go back to head-scratching reverse-engineering ;)
afacit and afair, we don't do any random indexing into that array or
anything like that - we always access just the head element and the
tail. It seems like a good fit for a list.
> Remember, this was an RFC.
Yes, I C'ed ;)
When a decent-looking patch has been lying fallow for over a month it's
time to kick it along a bit.
>
> > A pox on you for telling us there's a "potential starvation case" but
> > not telling us what it is! It's a kmalloc failure - why is this
> > described as "starvation"?
>
> Perhaps you missed that I simply moved that comment in the code; I
> didn't add it.
Oh, OK. A pox on you for not fixing it ;)
> When allocating an I/O context, the user supplies a
> maximum number of events. That number is used to size the ring buffer
> that holds completion data. It is possible that there is a completed
> request waiting for a final fput that is keeping a new allocation from
> succeeding. Calling the aio_fput_routine can free that up, allowing the
> allocation to succeed.
Oh well, if you can think of a way of clarifying and completing that
comment, please do so.
> > Once all this has been fixed, I might be wondering how we test the
> > "potential starvation case".
>
> Well, probably a close on the fd prior to event completion, coupled with
> submitting new I/O (on a different fd) when the event completion is
> delivered to userspace but before the fput on the fd.
Please put a printk in there and see if you can get it to go off?
> > Various trivial fixies:
> >
> >
> > From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> >
> > - coding-style fixes
> >
> > - improve variable name(s)
> >
> > - fix typo(s)
> >
> > - use new kmap_atomic() interface
> >
> > - change comment to use the number of columns God gave us
> >
> > - use "bool" for a boolean
> >
> > Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>
> This was an RFC. I'm happy to have it merged, but I would have been
> equally happy to have addressed your review comments first. In the end,
> whatever works best for you is fine with me.
>
I do think the patch (and the surrounding code) can be improved in
several ways.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
