Re: ADOSFS reading corrupted data

[Ilpo Ruotsalainen <lonewolf%iki.fi@localhost>]

On Sat Dec 21 2002 at 07:50:56 +0200, Ilpo Ruotsalainen wrote:
> > I tried to install NetBSD/amiga from 1.6 release CD image on my A1200
> > and ended up having same problems as described in PR 16021. After some
> > debugging it turned out that adosfs is reading corrupted data after
> > block boundary: on a FS with 1024 byte blocks the 1025th byte is read
> > wrong, on a FS with 4096 byte blocks the 4097th byte is read wrong.
> > 
> > Currently trying to figure out where things go wrong but as I lack
> > reasonable ways to transfer kernels to my amiga it'll probably take a
> > while, so if anyone can help in debugging this or already knows the
> > solution please let me know.
> 
> And the attached patch is the fix (works for me, YMMV but please someone
> test it before I commit it ;).

Attached is a more readable version of the patch (I should learn to wait
until I've slept before posting code...), I've verified that it works on
1kb and 4kb blocksize adosfs (FFS variant) filesystems.

> The bug was in the fact that adosfs scaled the block numbers one way in
> adosfs_read() and other way in adosfs_bmap(), which ended up with bad
> results when UBC called adosfs_bmap() and didn't scale the blocks like
> adosfs_read() did. Also mmap() on adosfs was broken because of the same
> 'feature' in adosfs_bmap().

A better description might be in order. What was happening was that (in
addition to) scaling the physical device reads as it should,
adosfs_read()/adosfs_bmap() was _also_ scaling the inside-the-file
virtual block numbers, so that reading the first 2 blocks from file on
4k blocksize FS would result in adosfs_bmap() being called with
in-the-file virtual blocks 0 and 8. To counter this, adosfs_bmap() was
undoing the scaling, so actually the whole end result for normal read()s
(without UBC) was contiguous block numbers. Enter mmap() and UBC:
adosfs_bmap() gets called with in-the-file virtual blocks 0 and 1 for
the first 2 blocks, both get internally scaled down to 0, thus mmap()
sees both the first FS blocks of the file content as the same block (and
with UBC read() suffers the same fate) and things go wrong, badly wrong.

My patch undoes the _part_ of rev 1.49 change that caused this
b0rkenness - but does not remove the scaling on the actual physical
device block numbers (which is what the change was trying to do, I
believe the scaling on internal in-the-file virtual block numbers was
unintentional). As mentioned abowe, I've confirmed that this works on 1k
and 4k blocksize filesystems on my Amiga, but it might be good if
someone else could test this too.

-- 
Ilpo Ruotsalainen - <lonewolf%iki.fi@localhost> - http://www.iki.fi/lonewolf/

Index: advnops.c
===================================================================
RCS file: /cvsroot/src/sys/adosfs/Attic/advnops.c,v
retrieving revision 1.62
diff --unified -r1.62 advnops.c
--- advnops.c   2001/11/12 22:59:18     1.62
+++ advnops.c   2002/12/24 00:52:11
@@ -311,8 +311,7 @@
                 * but not much as ados makes little attempt to 
                 * make things contigous
                 */
-               error = bread(sp->a_vp, lbn * amp->bsize / DEV_BSIZE,
-                             amp->bsize, NOCRED, &bp);
+               error = bread(sp->a_vp, lbn, amp->bsize, NOCRED, &bp);
                if (error) {
                        brelse(bp);
                        goto reterr;
@@ -481,7 +480,7 @@
        advopprint(sp);
 #endif
        ap = VTOA(sp->a_vp);
-       bn = sp->a_bn / (ap->amp->bsize / DEV_BSIZE);
+       bn = sp->a_bn;
        bnp = sp->a_bnp;
        if (sp->a_runp) {
                *sp->a_runp = 0;
@@ -572,7 +571,7 @@
        } else {
 #ifdef DIAGNOSTIC
                printf("flblk offset %ld too large in lblk %ld blk %d\n", 
-                   flblkoff, bn / (ap->amp->bsize / DEV_BSIZE), flbp->b_blkno);
+                   flblkoff, bn, flbp->b_blkno);
 #endif
                error = EINVAL;
        }
@@ -581,7 +580,7 @@
 #ifdef ADOSFS_DIAGNOSTIC
        if (error == 0 && bnp)
                printf(" %d => %d", bn, *bnp);
-       printf(" %d)", error);
+       printf(" %d)\n", error);
 #endif
        return(error);
 }