Re: Race condition in storeClose()

On 25 Nov 2000, at 2:17, Andres Kroonmaa <andre@online.ee> wrote:

> @@ -143,6 +143,8 @@
> if (storeAufsSomethingPending(sio)) {
> + if (aiostate->flags.reading)
> + aioCancel(aiostate->fd);

 I'm still having crashes with aioCancel applied. Seems that problem is
 not only in that storeClose doesn't cancel aioRead, but also in the fact
 that storeClose isn't called timely at all.

 To me it seems that there are zillions of places in client_side.c
 where client socket buffer is released without making sure that pending
 aioread is canceled. Basically every place where we release client buff,
 we should first cancel pending read with storeunregister, or have
 clientbuffer cblocked, so that its final free is done during aiocallback
 checks.
 This part of squid is so complex that I'm very unsure about where and
 what should we do with that.

 As I see, storeAufsRead is doing cbdataLock(callback_data), and
 storeAufsReadDone is doing cbdataUnlock(their_data). This should be
 good enough safeguard against freeing buffer. Then it shold be
 enough to fix this bug if we used cbdataFree instead of memFree on the
 socket buff, and have it pushed onto cbdata during alloc.

 (Interestingly, storeAufsWrite is not bothering with cblocks. Seems
  like someone has forseen this aioRead problem, but have forgotten to
  handle it.)

 We could in aio_cleanup_request check or assert that cbdataValid(their_data)
 holds true before copying aio data back to caller. If cbdata isn't valid,
 we can assume canceled request (buffer is gone).

 Perhaps if we can detect that client buffer has gone at copyback
 time, we don't even need to bother canceling the read in storeClose.
 Maybe makes it easier with diskd, which I understand doesn't have
 any ioCancel method.

 Is putting client socket buffer onto cbdata the right way to go?

> guys, please try this patch who is using aufs.
> with it, I'm having less crashes ;), and those that come
> seem to be something else...
>
> Index: store_io_aufs.c
> ===================================================================
> RCS file: /cvsroot/squid/squid/src/fs/aufs/store_io_aufs.c,v
> retrieving revision 1.3
> diff -u -r1.3 store_io_aufs.c
> --- store_io_aufs.c 2000/11/11 09:40:03 1.3
> +++ store_io_aufs.c 2000/11/24 21:42:51
> @@ -143,6 +143,8 @@
> sio->swap_dirn, sio->swap_filen, aiostate->fd);
> if (storeAufsSomethingPending(sio)) {
> + if (aiostate->flags.reading)
> + aioCancel(aiostate->fd);
> aiostate->flags.close_request = 1;
> return;
> }
> storeAufsIOCallback(sio, DISK_OK);

------------------------------------
 Andres Kroonmaa <andre@online.ee>
 Delfi Online
 Tel: 6501 731, Fax: 6501 708
 Pärnu mnt. 158, Tallinn,
 11317 Estonia
Received on Mon Nov 27 2000 - 13:33:12 MST