DragonFlyBSD

Sascha Wildner wrote:

After discussing things on IRC with corecode, he came up with a fix that
works for me (see the followup to this mail).

I noticed two unrelated bugs, which are both fixed in the following patch:

1. rbp is geteblk()'ed, but in case of a VOP_BMAP() error returned without B_INVAL set.

2. I moved the read-ahead after starting the real read for bp, so that I can skip this if an error occured in the reading of bp. Before rbp might have been a CLUSTER and didn't get special treatment for freeing (hence the panic).

cheers
simon

diff r 44daa8fb3603 sys/kern/vfs_cluster.c
-- a/sys/kern/vfs_cluster.c Sun Jul 09 03:36:13 2006 0200
++ b/sys/kern/vfs_cluster.c Sun Jul 09 21:34:01 2006 +0200
@ -200,46 +200,6 @ single_block_read:
}

/*
-     * If we have been doing sequential I/O, then do some read-ahead.
-     /
-    rbp = NULL;
-    if (seqcount &&
-        loffset < origoffset + seqcount * size &&
-        loffset + size <= filesize
-    ) {
-        rbp = getblk(vp, loffset, size, 0, 0);
-        if ((rbp->b_flags & B_CACHE) == 0) {
-            int nblksread;
-            int ntoread;
-            int burstbytes;

            error = VOP_BMAP(vp, loffset, NULL,
-                     &doffset, &burstbytes, NULL);
-            if (error || doffset == NOOFFSET) {
-                brelse(rbp);
-                rbp = NULL;
-                goto no_read_ahead;
-            }
-            ntoread = burstbytes / size;
-            nblksread = (totread + size - 1) / size;
-            if (seqcount < nblksread)
-                seqcount = nblksread;
-            if (seqcount < ntoread)
-                ntoread = seqcount;

            rbp->b_flags |= B_RAM;
-            if (burstbytes) {
-                rbp = cluster_rbuild(vp, filesize, loffset,
-                             doffset, size, 
-                             ntoread, rbp, 1);
-            } else {
-                rbp->b_bio2.bio_offset = doffset;
-            }
-        }
-    }
no_read_ahead:

-    /
 * Handle the synchronous read.  This only occurs if B_CACHE was
 * not set.  bp (and rbp) could be either a cluster bp or a normal
 * bp depending on the what cluster_rbuild() decided to do.  If
@ -262,39 +222,72 @ no_read_ahead:
     }

/*
-     * And if we have read-aheads, do them too. 
-     /
-    if (rbp) {
-        if (error) {
+     * If we have been doing sequential I/O, then do some read-ahead.
+     */
+    rbp = NULL;
+    if (!error &&
+        seqcount &&
+        loffset < origoffset + seqcount * size &&
+        loffset + size <= filesize
+    ) {
+        int nblksread;
+        int ntoread;
+        int burstbytes;

        rbp = getblk(vp, loffset, size, 0, 0);
+        if ((rbp->b_flags & B_CACHE)) {
+            bqrelse(rbp);
+            goto no_read_ahead;
+        }

        error = VOP_BMAP(vp, loffset, NULL,
+                 &doffset, &burstbytes, NULL);
+        if (error || doffset == NOOFFSET) {
+            rbp->b_flags |= B_INVAL;
             brelse(rbp);
-        } else if (rbp->b_flags & B_CACHE) {
-            bqrelse(rbp);
+            rbp = NULL;
+            goto no_read_ahead;
+        }
+        ntoread = burstbytes / size;
+        nblksread = (totread + size - 1) / size;
+        if (seqcount < nblksread)
+            seqcount = nblksread;
+        if (seqcount < ntoread)
+            ntoread = seqcount;

        rbp->b_flags |= B_RAM;
+        if (burstbytes) {
+            rbp = cluster_rbuild(vp, filesize, loffset,
+                         doffset, size, 
+                         ntoread, rbp, 1);
         } else {
+            rbp->b_bio2.bio_offset = doffset;
+        }
 #if defined(CLUSTERDEBUG)
-            if (rcluster) {
-                if (bp)
-                    printf("A+(lld,%d,%lld,%d) ",
-                        rbp->b_loffset, rbp->b_bcount,
-                        rbp->b_loffset - origoffset,
-                        seqcount);
-                else
-                    printf("A(%lld,%d,%lld,%d) ",
-                        rbp->b_loffset, rbp->b_bcount,
-                        rbp->b_loffset - origoffset,
-                        seqcount);
-            }
+        if (rcluster) {
+            if (bp)
+                printf("A+(%lld,%d,%lld,%d) ",
+                    rbp->b_loffset, rbp->b_bcount,
+                    rbp->b_loffset - origoffset,
+                    seqcount);
+            else
+                printf("A(%lld,%d,%lld,%d) ",
+                    rbp->b_loffset, rbp->b_bcount,
+                    rbp->b_loffset - origoffset,
+                    seqcount);
+        }
 #endif
-            rbp->b_flags x%x= ~(B_ERROR|B_INVAL);
-            rbp->b_flags |= B_ASYNC;
-            rbp->b_cmd = BUF_CMD_READ;

            if ((rbp->b_flags & B_CLUSTER) == 0)
-                vfs_busy_pages(vp, rbp);
-            if ((rbp->b_flags & B_ASYNC) || rbp->b_bio1.bio_done != NULL)
-                BUF_KERNPROC(rbp);
-            vn_strategy(vp, &rbp->b_bio1);
-        }
-    }
+        rbp->b_flags &= ~(B_ERROR|B_INVAL);
+        rbp->b_flags |= B_ASYNC;
+        rbp->b_cmd = BUF_CMD_READ;

        if ((rbp->b_flags & B_CLUSTER) == 0)
+            vfs_busy_pages(vp, rbp);
+        BUF_KERNPROC(rbp);            / B_ASYNC */
+        vn_strategy(vp, &rbp->b_bio1);
+    }
no_read_ahead:

     if (reqbp)
         return (biowait(reqbp));
     else

:I noticed two unrelated bugs, which are both fixed in the following patch=
::
:
:1. rbp is geteblk()'ed, but in case of a VOP_BMAP() error returned withou=
:t B_INVAL set.

I would say that this is definitely a bug.  As per the code comments
    for getblk().

:2. I moved the read-ahead after starting the real read for bp, so that I =
:can skip this if an error occured in the reading of bp. Before rbp might=
: have been a CLUSTER and didn't get special treatment for freeing (hence =
:the panic).
:
:cheers
: simon

I don't think checking 'error' here is going to matter.  In fact,
    I think the error assignment in the original code is probably wrong:

vn_strategy(vp, &bp->b_bio1);
    error = bp->b_error;        <<<<<< HERE

Most drivers set b_error when the I/O completes rather then from
    vn_strategy(), so error is most likely going to be 0 here.

However, I don't want to change too much at once so lets just leave
    it as it is in your patch.

I will run some tests with this patch, but it looks pretty good.

-Matt

:I got a panic here which occured after I did:
:
:# vnconfig c -v /dev/vn0 dvd.iso
:# mount_cd9660 -o ro /dev/vn0 /mnt
:# cat /mnt/* >/dev/null
:
:After discussing things on IRC with corecode, he came up with a fix that
:works for me (see the followup to this mail).
:
:But now I get "File too large" for a couple of files (which are around
:660MB in size) on the ISO, but it no longer panics.
:
:I'm still not sure if "cd9660" is the right type for a DVD. Can anyone
:confirm/deny that?
:
:Sascha
:
:P.S. For the interested, a dump is in ~swildner/crash/brelse.tbz on leaf.
:
:-
:http://yoyodyne.ath.cx

Is Simon's cluster bug fix the one you are talking about here ?

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Matthew Dillon wrote:

I don't think checking 'error' here is going to matter. In fact,
I think the error assignment in the original code is probably wrong:

vn_strategy(vp, &bp->b_bio1);
error = bp->b_error; <<<<<< HERE

Most drivers set b_error when the I/O completes rather then from
vn_strategy(), so error is most likely going to be 0 here.

I thought so as well, but: otherwise sascha's panic would never have occured, so error must have been set already.

cheers
simon

Matthew Dillon wrote:

:I got a panic here which occured after I did:
:
:# vnconfig c -v /dev/vn0 dvd.iso
:# mount_cd9660 -o ro /dev/vn0 /mnt
:# cat /mnt/* >/dev/null
:
:After discussing things on IRC with corecode, he came up with a fix that
:works for me (see the followup to this mail).
:
:But now I get "File too large" for a couple of files (which are around
:660MB in size) on the ISO, but it no longer panics.
:
:I'm still not sure if "cd9660" is the right type for a DVD. Can anyone
:confirm/deny that?
:
:Sascha
:
:P.S. For the interested, a dump is in ~swildner/crash/brelse.tbz on leaf.
:
:-
:http://yoyodyne.ath.cx

Is Simon's cluster bug fix the one you are talking about here ?

Yes, it's fixed by Simon's patch.

Sascha

Matthew Dillon wrote:

I will run some tests with this patch, but it looks pretty good.

should I commit this patch or will you?

cheers
simon

:
:This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
:--------------enig5938307185E36A8CAB65A70B
:Content-Type: text/plain; charset=UTF-8; format=flowed
:Content-Transfer-Encoding: quoted-printable
:
:Matthew Dillon wrote:
:> I will run some tests with this patch, but it looks pretty good.
:
:should I commit this patch or will you?
:
:cheers
: simon

The vinum patch?  The trypbuf()'s need to be turned into getpbuf()'s.
    That's the only problem.

-Matt

Matthew Dillon wrote:

:
:This is an OpenPGP/MIME signed message (RFC 2440 and 3156)
:--------------enig5938307185E36A8CAB65A70B
:Content-Type: text/plain; charset=UTF-8; format=flowed
:Content-Transfer-Encoding: quoted-printable
:
:Matthew Dillon wrote:
:> I will run some tests with this patch, but it looks pretty good.
:
:should I commit this patch or will you?
:
:cheers
: simon

The vinum patch? The trypbuf()'s need to be turned into getpbuf()'s.
That's the only problem.

ah no, the vinum patch was already committed with geteblk() (how it was) instead.

this one is the cluster_read() thing where in a rare error case (vnstrategy directly fails) a getpbuf() is returned with a brelse()

cheers
simon

:ah no, the vinum patch was already committed with geteblk() (how it was) =
:instead.
:
:this one is the cluster_read() thing where in a rare error case (vnstrate=
:gy directly fails) a getpbuf() is returned with a brelse()
:
:cheers
: simon

I must have de-applied it from my test tree when I did the vinum
    testing.  I'll throw it back in and test it through today.

I think you can go ahead and commit it now, though.   If any problems
    show up in my testing we can always back it out.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>