I need to analyze async-io on close a bit more, but my initial
reaction is this patch (both versions) can't be correct but quite
likely hides a real problem.
Problem: aioRead() will read into the supplied buffer even if the core
level swapin operation has been aborted.
Why: storeClose() is a async operation, and is allowed to be called
when there is pendin I/O operations. When there is pending I/O
operations the close will finish after the last I/O operation.
Problem with the above: The Squid core use of storeClose() is slightly
overloaded. It is also used with no indication of difference when the
I/O operations should be aborted. This causes a slight problem on
swapins.
The proposed patches probably acheives the correct effect for swapins
or aborted objects, but not for swapouts. On swapout the sio is
closed when the last piece of data has been scheduled for swapout,
and not accounting for this will randomly truncate swapouts.
Diskd seems to be safe in this regard as the copying into the supplied
buffer is there done at a higher level being cbdata aware, but there
might be corner cases..
aufs whould hightly benefit from the refcounted I/O buffers we have
been talking about, but at a minimum it should get rid of the middle
indirection layer to allow for sane cbdata operations allowing
storeAufsRead() to cbdata verify the caller before copying into the
supplied buffer..
As a middle step, introducing a storeIoAbort() FS operation would help
to address the issue.. or as a quick fix emulating storeIoAbort() on
close of a read-only aufs SIO.
Regards
Henrik
On Monday 04 November 2002 04.29, Adrian Chadd wrote:
> On Mon, Nov 04, 2002, Robert Collins wrote:
> > > Any suggestions on where to look in the 2.5 branch?
> >
> > I've not yet reviewed in detail the attached patch, but it looks
> > good to me at first glance.
>
> Ooer.
>
> > After weeks of poking around in the code, I've finally figured
> > out what was causing download corruption in Squid 2.4 and 2.5.
>
> Whoa. This person is .. insane. Insane, insane, insane.
> I wonder if diskd suffers from this at all.
>
> > Although I never heard back from the squid developers as to
> > whether they were working on this, hopefully they will have
> > something to say now that it has (ostensibly) been solved.
> > Hello...anybody there?
>
> Grr. He never sent it to squid-dev? :)
>
> > -Phil Oester
> > (not a squid developer, but I did sleep at a Holiday Inn express
> > last night)
>
> How does this make sense? :)
>
> I'm happy with the following patch. It makes sense to me. Does
> anyone have any issues with it?
>
> > --- original/src/fs/aufs/store_io_aufs.c Sun Aug 11
> > 16:14:35 2002 +++ fixed/src/fs/aufs/store_io_aufs.c Mon Oct 28
> > 14:39:32 2002 @@ -286,6 +286,10 @@
> > aiostate->fd = fd;
> > commSetCloseOnExec(fd);
> > fd_open(fd, FD_FILE,
> > storeAufsDirFullPath(INDEXSD(sio->swap_dirn), sio->swap_filen,
> > NULL)); + if (aiostate->flags.close_request) {
> > + storeAufsIOCallback(sio, errflag);
> > + return;
> > + }
> > if (FILE_MODE(sio->mode) == O_WRONLY) {
> > if (storeAufsKickWriteQueue(sio))
> > return;
> > @@ -293,8 +297,6 @@
> > if (storeAufsKickReadQueue(sio))
> > return;
> > }
> > - if (aiostate->flags.close_request)
> > - storeAufsIOCallback(sio, errflag);
> > debug(79, 3) ("storeAufsOpenDone: exiting\n");
> > }
Received on Mon Nov 04 2002 - 01:36:27 MST
This archive was generated by hypermail pre-2.1.9 : Tue Dec 09 2003 - 16:18:37 MST