SVN rev 325: Trap D on booting to a commandline and running "lvm /rediscoverprm"

[erdmann]

SVN rev 325:

On boot, I hit Alt-F1 and then F2 to get me to a commandline. I then ran "lvm /rediscoverprm" to get access to my plugged in USB devices, some of those formatted to FAT32. That lead to the following trap (and this trap is always reproducible at the very same EIP offset). I did not have a camera ready, therefore I copied this manually:

Trap D in FAT32.IFS

ERRCD=0000 ERACC=**** ERLIM=******** CPU=01
EAX=00000000 EBX=0000000b ECX=fd0f000b EDX=fd2f0000
ESI=fd2f0000 EDI=2bcb0000 EBP=00004d20 FLG=00210206
CS:EIP=3110:000064c0 CSACC=009b CSLIM=0000fdcc
SS:ESP=1530:00004cb6 SSACC=1097 SSLIM=0000410f
DS=3128 DSACC=0093 DSLIM=00007617 CR0=8001003b
ES=1530 ESACC=1097 ESLIM=0000410f CR2=fa8e2000
FS=0000 FSACC=**** FSLIM=********
GS=0000 GSACC=**** GSLIM=********

What I already tried: I moved the IFS=FAT32.IFS line further up in config.sys as it was located at the end of CONFIG.SYS (I thought that that might have an influence). However that did not help, I got the very same trap on attempting to do the same.

[erdmann]

Tested with SVN rev 322: this error does not occur with SVN rev 322

[Valery V. Sedletski]

What I didn't understood is why you press Alt-F1->F2. Do you do "lvm /rediscoverprm" in command line? Does it trap on usual boot? Why Alt-F1->F2 does matter?

Between r322 and r325, I only changed a FAT read/write portion size to 64 sectors on non-FAT12 FS. I doubt that it may cause traps. So, I don't know why r322 is not trapping for you.

PS: I saw traps or hangs when accessing LVM engine, but it was because of LVM bugs, I think.

[Valery V. Sedletski]

Just tried Alt-F1->F2, did "lvm /rediscoverprm" I see a new drive letter of my flash disk appeared, no trap.

[erdmann]

Because of problems I had to boot to a commandline. Then I needed to access USB stick which required "lvm /rediscoverprm". Is that explanation enough ?

No it does not trap on normal boot. But it eventually hangs the whole system, see my other ticket.

Just run "lvm /rediscoverprm" until it crashes. It eventually will.

Why don't you just look at the trap screen and find the place in the code ? It shouldn't be too hard with a MAP file at hand.

[Valery V. Sedletski]

Ok, disassembled it, with help of a map file. It looks like it is trapping on FS_MOUNT in the following assembly fragment:

.000264A3: 9A00000000                   call   00003:000008B40 ---------- (1)
.000264A8: 83C404                       add    sp,004
.000264AB: 8B4ED4                       mov    cx,[bp][-002C]
.000264AE: 8B5ED6                       mov    bx,[bp][-002A]
.000264B1: 31D2                         xor    dx,dx
.000264B3: 31C0                         xor    ax,ax
.000264B5: 39C3                         cmp    bx,ax
.000264B7: 7504                         jne   .0000264BD   ---------- (2)
.000264B9: 39D1                         cmp    cx,dx
.000264BB: 741A                         je    .0000264D7   ---------- (3)
.000264BD: C45ED4                       les    bx,[bp][-002C]
.000264C0: 268B5708                     mov    dx,es:[bx][00008]

At the very end (the last "mov" instruction). It corresponds to the following source fragment:

         if (pDevCaps->Capabilities & GDC_DD_16M)
            Message(">16M supported");
         }

      if (pDevCaps && pDevCaps->Strategy2)
         {
         if (f32Parms.fMessageActive & LOG_FS)
            {
            Message("Strategy2   address at %lX", pDevCaps->Strategy2);
            Message("ChgPriority address at %lX", pDevCaps->ChgPriority);
            }

It traps on pDevCaps->Strategy2, where pDevCaps is previously checked to be != 0. So, pDevCaps has a wrong value != 0 (Probably, an invalid selector). pDevCaps is taken from pvpfsi->vpi_pDCS, so I doubt that it is the IFS is hurting the value. Most probably, it is an .ADD driver (usbmsd.add). Which usbmsd.add version are you using? Could you try testing with an old IBM's USBMSD.ADD 10.062 or similar?

PS: And please, recheck issue #65 too, with r325/r326 and IBM's USBMSD too (will it hang?). As I don't encounter such issues on my system with r325/r326, something is wrong with your system. I suspect that it may be an USB stack. (I use USBMSD 10.216 with IBM's USB stack just fine, so, maybe, something could happen with latter USB stacks)

[erdmann]

Ok, so I understand that the code loads "pDevCaps" far pointer into ES:BX (at code Location 264BD), correct ?
Therefore, do you know if "Strategy2" can be found at Offset 8 in the structure pointed to by "pDevCaps" (I will need to check that ...).

But if all that is true, how can ES take on the value of SS ? Shouldn't that pointer "pDevCaps" be passed in from the kernel ? Something is very odd here.

[erdmann]

Hi Valery:

pDevCaps = pvpfsi->vpi_pDCS;

This means, pDevCaps is very unlikely to take on the value of a stack address. You must have been looking at the wrong place in the code.
Or: I am beginning to believe we are now running into the same problem that I fixed years ago: pDevCaps is invalid and needs to be reread from OS2DASD.DMD.
That's exactly the code you commented out in file "ifsmount.c":

         //if (!pDevCaps) 
         //   { 
         //   Message("Strategy2 not found, searching Device Driver chain !"); 
         //   pDevCaps = ReturnDriverCaps(pvpfsi->vpi_unit); 
         //   } 

[Valery V. Sedletski]

No, this code is commented out, because it is wrong! Many .ADD drivers have no strat2 routine. For example this is the case with ibm1flpy.add, or with the .ADD driver from QSINIT for PAE ramdisk, if the "/1" switch is specified. The strat1 only mode is designed for better performance. PAE ramdisk with .ADD driver in strat1 only mode works faster than with strat2. In such case the above commented code wrongly detects strat2, when it is missing. It is strat2 from OS2DASD.DMD, it seems, but we need strat2 from a specific .ADD driver, which is missing.

Yes, pDevCaps is loaded into es:bx and then it's tried to use a pointer in es:bx to reference pDevCaps->Strategy2, thus trapping (probably, because of an invalid selector). pDevCaps is taken from pvpfsi->vpi_pDCS. If it is == 0, then strat2 is missing. But in this case, pointer != 0, it is just invalid.

But if all that is true, how can ES take on the value of SS ? Shouldn't that pointer "pDevCaps" be passed in from the kernel ? Something is very odd here.

What do you mean? The les bx,[bp][-002C] command means loading ss:[bp][-002C] into es:bx. Yes, pDevCaps is taken from struct vpfsi, which is passed by kernel to the IFS. But kernel probably, gets it from the corresponding .ADD, I suppose.

[Valery V. Sedletski]

 Hi Valery:

    pDevCaps = pvpfsi->vpi_pDCS;

This means, pDevCaps is very unlikely to take on the value of a stack address.

Why do you think it should be a stack address? Yes, it is device capabilities structure, taken from .ADD data, if I correctly understand.

Or: I am beginning to believe we are now running into the same problem that I fixed years ago: pDevCaps is invalid and needs to be reread from OS2DASD.DMD.

Why do you think it should be reread from OS2DASD? We need to understand, why it is invalid. And why I am not getting this error on my machine? (It looks that pDevCaps is normal). I'd suppose it could be the bug in an .ADD driver (e.g., USBMSD one).

[Valery V. Sedletski]

This means, pDevCaps is very unlikely to take on the value of a stack address.

Ah, pDevCaps is a variable on stack, yes. It is FS_MOUNT local variable, and its value is got from pvpfsi->vpi_pDCS, just copied. pvpfsi->vpi_pDCS itself is not.

[erdmann]

The CONTENT of pDevCaps ends up in ES:BX and points to the stack because the trap screen shows that ES == SS ! In other words: pvpfsi->vpi_pDCS is a stack address and THAT is definitely wrong !
And reading the pDCS via an OS2DASD.DMD dedicated strat1 command "GetDeviceCaps" (command code: 0x1D) is NOT wrong and I was NOT using the strat2 entry point to do it. I was using the strat1 entry point of OS2DASD.DMD to do it ! Please have a look at routine "ReturnDriverCaps" that you commented out.

I have the impression that you misunderstand what "ReturnDriverCaps" does and how it operates.

As I said I ran into this problem years ago. I was using PPAOS2.ADD (the driver for using Iomega parallel port controlled ZIP drives). And I was using a ZIP media formatted as FAT32.
What happened, just like this time, is that pDCS was invalid. Back at the time it was a NULL pointer. Now, it looks like it can be an arbitrarily wrong pointer. I strongly suggest to ALWAYS use "ReturnDriverCaps" to get the pDCS pointer. This looks like a kernel bug.

[Valery V. Sedletski]

The CONTENT of pDevCaps ends up in ES:BX and points to the stack because the trap screen shows that ES == SS ! In other words: pvpfsi->vpi_pDCS is a stack address and THAT is definitely wrong !

Agree.

But even if that your routine retrieves is strat1, then why I see then

pVolInfo->pfnStrategy = (STRATFUNC)pDevCaps->Strategy2

Why are we sure that this driver has strat2? It may be missing indeed so making the above assignment is wrong (even if finding pDevCaps your way is correct). It was like that in version 0.9.13 days, and now fat32.ifs can work without strat2 (due to cache code changes made by Ko Myung Hun) and can work on different media which can have no strat2.

As I said I ran into this problem years ago. I was using PPAOS2.ADD (the driver for using Iomega parallel port controlled ZIP drives). And I was using a ZIP media formatted as FAT32. What happened, just like this time, is that pDCS was invalid. Back at the time it was a NULL pointer. Now, it looks like it can be an arbitrarily wrong pointer.

So, in the older days, it was 0, but later it becomes invalid? And it was with the Iomega .ADD? So, this does not only happen with USBMSD.ADD?

I strongly suggest to ALWAYS use "ReturnDriverCaps?" to get the pDCS pointer. This looks like a kernel bug.

It is possible too, of course.

[Valery V. Sedletski]

Also, the following question: how the code fragment

if (!pDevCaps)
   {
   Message("Strategy2 not found, searching Device Driver chain !");
   pDevCaps = ReturnDriverCaps(pvpfsi->vpi_unit);
   }

can help us? pDevCaps is not zero, so it won't help us avoid the trap later when using pDevCaps pointer. Or we need to remove check for "if (!pDevCaps)" ? Is it sane to do this?

[erdmann]

The assignment is not wrong. But it is possible that pVolInfo->pfnStrategy will be NULL.
But you are right, if we do not use what pDevCaps returns anyway, we can get rid of querying pDevCaps altogether. You will need to look around in the code if we need the contents of pDevCaps at all (or only on FS_MOUNT to print out uninteresting bullshit).

Yes, testing for "!pDevCaps" won't help. We would need to call "ReturnDriverCaps" in ANY case to be sure to get a valid pDevCaps pointer. In other words: "pvpfsi->vpi_pDCS" contains a completely unreliable/bogus/wrong value.

Also, all this crap is not a problem of the ADDs but rather of the DMD that manages the ADDs. Here is how it works:

1) if a volume is on a media managed by an ADD (as almost always is the case) then the corresponding DMD (almost always OS2DASD.DMD except for CD-ROMs where it is OS2CDROM.DMD) will receive requests from the upper layers (IFS) to do something.
The DMD has a strat1 entry point but might also offer a strat2 (extended strat1 supporting request lists) and even a strat3 (32-bit) entry point (JFS.IFS uses the 32-bit strat3 entry point for optimal throughput).
A caller (the IFS) can query if strat2 and strat3 support exists by doing a "GetDevCaps" (code: 0x1D) against the strat1 entry point of the DMD.
The DMD will return with the DevCaps structure (in addition, the IFS should receive this info via the kernel on the FS_MOUNT call but as you see that is broken).
It is the callers responsibility to ensure that a strat2 or strat3 entry point is available if it intends to use it !

2) on a data transfer request/management request/etc. the DMD will invoke the ADD managing the media via the ADD's IORB entry point. The ADD will do what is necessary and return to the DMD.

3) if the volume is on a media managed by an old legacy type block driver (like vdisk.sys or vfdisk.sys), then the block driver will receive all requests directly via its strat1 entry point without the involvement of a DMD.
If it has a strat2 entry point, again, this can be queried by an upper layer by invoking the strat1 entry point by doing a "GetDevCaps" call.
Again, the upper layer has to check if the strat2 entry point is available before it attempts to use it. Legacy type block drivers do not export a strat3 entry point (at least I know of none that would do that).

[Valery V. Sedletski]

Thanks for the note about GetDevCaps and how to check for presence of start2/strat3, if strat1 is present. But how could I find a strat1 address of an .ADD then? Yes, most probably, the .ADD is managed by OS2DASD.DMD, but OPTICAL.DMD can be the case of magnetooptical drives, and OS2CDROM.DMD too (as you could notice, I was able to read/write to a FAT formatted CDRW, and it works). Strat2 is needed 1) for cache to flush sectors to disk, read them (but atm, Ko Myung Hun removed strat2 for unknown reason, and used FSH_DOVOLIO instead. This should degrade performance.) 2) to swap, if we're using FS_DOPAGEIO (I have unfinished swap routines in ifsswap.c). So, strat2 found via pDevCaps, is used for these two purposes. If getting it via pvpfsi->vpi_pDCS is unreliable, how can I get them for each .DMD then? Could I reuse the ReturnDriverCaps rounine? I'll need also to detect which .DMD is used in particular case.

[erdmann]

"Finding strat2": yes, you have hit the problem. Currently, "ReturnDriverCaps" will only return strat2/strat3 for OS2DASD.DMD and nothing else. It will neither return strat2/strat3 for OPTICAL.DMD nor for OS2CDROM.DMD. It will also not return strat2/strat3 for any old legacy block device driver like VFDISK.SYS and VDISK.SYS.
What you would need to do is issue the "GetDevCaps" request against EVERY driver in the driver chain, passing it the unit id (as is done in ReturnDriverCaps). You should only get exactly 1 resulting driver that answers with "NO_ERROR". That will be a DMD or a block device driver and it will be the one that manages the unit you were asking for. Of course you would only do that call exactly once on each FS_MOUNT (you don't need to do that over and over again).
In short that means to remove this check in "ReturnDriverCaps":

      if (
          (memicmp(&pDD->SDevName[1],"Disk DD",7) == 0) &&        // found OS2DASD.DMD
          (pDD->SDevAtt == (DEV_NON_IBM | DEVLEV_1 | DEV_30))     // found OS2DASD.DMD
         )

and instead check if pRH->rph.Status == STDON. Something like this:

   while (pDD && (pDD != (struct SysDev far *)-1))
      {
         pStrat = (PFN)MAKEP(pDD->SDevProtCS,pDD->SDevStrat);
         dsSel   = pDD->SDevProtDS;

         if (pStrat)
            {

            pRH->rph.Len    = sizeof(rp);
            pRH->rph.Unit   = ucUnit;
            pRH->rph.Cmd    = CMDGetDevSupport;

            _asm
               {
               push ds
               push es
               push bx
               mov ax,dsSel
               mov ds,ax
               mov es,word ptr pRH+2
               mov bx,word ptr pRH
               call dword ptr pStrat
               pop bx
               pop es
               pop ds
               }
            if (pRH->rph.Status == STDON)
               {
               return pRH->pDCS;
               }
            }
         pDD = (struct SysDev far *)pDD->SDevNext;
      }

and hope that no driver will return STDON if it does not support the "GetDevCaps" call (it should not return STDON then).

But I don't see why we need strat2 at all. For transfers, the DMD will do whatever is necessary to "transform" the read/write request into one or more proper IORB requests (IOCC_EXECUTE_IO,IOCM_READ|IOCM_READ_VERIFY|IOCM_READ_PREFETCH|IOCM_WRITE|IOCM_WRITE_VERIFY) and then call the ADD. This is true, regardless if OS2DASD.DMD is invoked via its strat1 or strat2 or strat3 entry point. Initially IBM came up with a strat2 entry point to improve performance of the old legacy block device drivers. But these were mostly replaced by the new DMD/ADD architecture. It is now the DMD that contains the relevant strat1/strat2/strat3 entry points. You can still use the strat1 READ and WRITE request of the DMD to read/write data.
I would think that FSH_DOVOLIO is just a wrapper around the DMD/legacy device driver strat1 entry point and nothing more (so that you don't have to find out WHICH DMD/legacy device driver needs to be called. FSH_DOVOLIO will find the correct DMD/block driver via the VPB info).

Please note: strat2 and strat3 entry points are OPTIONAL. You cannot rely on their existence. In short: you still might be forced to use the strat1 entry point/FSH_DOVOLIO.
That will certainly be true for a volume managed by VFDISK.SYS as it is a legacy block device driver that does not support the strat2 entry point.

Don't worry too much about performance. The largest performance hit is FAT32.IFS itself with its inefficient sector handling.

You will never need to find a strat1 entry point of an ADD. The strat1 entry point of an ADD is only used on BASEDEVINIT to set up initial stuff,print banner, etc. and possibly for special/proprietary IOCTL requests. It is not used for anything else. In particular, it is not called on a data transfer. The main entry point of an ADD is the IORB entry point.

[erdmann]

Just checked: OS2CDROM.DMD only contains the strat1 entry point. It does not support the "GetDevCaps" request and therefore does not support the strat2 or strat3 entry points.

Same holds true for OPTICAL.DMD.

[Valery V. Sedletski]

Heh, Just noticed. It appears that pDevCaps remains uninitialized (look at the beginning of FS_MOUNT, where it is declared. It should be initialized to NULL here), if

if (!pDevCaps)
   {
   pDevCaps = ReturnDriverCaps(pvpfsi->vpi_unit)
   }

is commented out. That is because we got a trap with a non-zero pDevCaps. I suspect that this is not a kernel bug, but we just forgot to initialize it to NULL :) All problems are because of this.

[Valery V. Sedletski]

Ah, I'm wrong. Yes, there is

pDevCaps = pvpfsi->vpi_pDCS;

which I forgot about. So, yes, the problem persists.

[Valery V. Sedletski]

strat2/strat3 are needed not only for enhancing performance of pre-DMD old-style block drivers. They enhance performance of newer-style device drivers too. strat1 is not needed by itself. Currently, FSH_DOVOLIO is used for read/write requests, which calls strat1 under the hood. Strat2 is needed for better performance, to use scatter/gather lists, which are needed to run I/O requests in batches, at once. It is needed very much for flushing the cache and do swapping requests. I tried swapping with FSH_DOVOLIO, and it is *very* slow. I ran swapping on a HPFS ramdisk (QSINIT one), with strat1 and strat2. With strat2, it works good, as usual. But if you call FSH_DOVOLIO (which calls strat1 in turn), the system is inresponsive and 100% CPU load. This way it works without the scatter/gather feature. strat2 is disabled in QSINI's .ADD driver by specifying the "/1" parameter. With it, IFS'es like HPFS own't find the strat2 entry point and will use strat1 instead. With strat1, HPFS performance on a RAM disk in about 10 times better (because of some simplification). But swapping performance, in particular, very much depends on scatter/gather, because it uses much page-size requests, which can cause a significant slowdown in case of their serialization. But they work much faster when doing asynchronously.

What do you mean by inefficient sector handling in fat32.ifs? Yes, I agree that it is very slow compared with intended performance, like in Linux/Windows. In fact, it is about 10 times slower on large transfers (copying big files). Also, at present, the cache is disabled. So, there is no performance enhancements caused by cache. To have efficient caching, we need efficient big file copying at first (which can work fast even without a cache. A cache enhances performance on small scattered data, at most). But for enhancing copying big files, we'll need to analyse the bottlenecks. We'll need to add performance counters support, I think. This will allow to gather statistics. And based on this statistics, we could decide about what to enhance.

[erdmann]

As I already said, you cannot rely on every DMD to support the strat2/strat3 entry point. Therefore an IFS always needs a fallback solution to use strat1.
I think JFS.IFS also implements a fallback to using strat1 if strat3 is not available (JFS.IFS never uses strat2).
I have no idea of how the QSINIT RAM disk ADD driver can control of what strat entry point is offered (by supporting the /1 switch).
As I said, it's the DMDs (or legacy block drivers) that offer the strat1/strat2/strat3 entry points and not the ADD.

With "inefficient sector handling" I mean that FAT32.IFS does a bad job in combining requests to read/write adjacent sectors/clusters. I once implemented this "adjacent sector/cluster combining" to improve read/write performance when calling FSH_DOVOLIO and in fact it worked.
Unfortunately that had to be removed because of some high memory issue, see:

"ifsfile.c", FS_READ and FS_WRITE:

#if 0 
                    /* 
                        The following code is fast, but is not compatible 
                        with OBJ_ANY attribute 
                    */ 
                    rc = ReadSector(pVolInfo, ulClusterSector,usSectorsToRead,pBufPosition, usIOFlag); 
                    if (rc) 
                    { 
                        goto FS_READEXIT; 
                    } 
                    pBufPosition                    += (USHORT)ulCurrBytesToRead; 
                    pos                             += (USHORT)ulCurrBytesToRead; 
                    usBytesRead                     += (USHORT)ulCurrBytesToRead; 
                    usBytesToRead                   -= (USHORT)ulCurrBytesToRead; 
                    usAdjacentClusters              = 1; 
                    pOpenInfo->ulCurCluster         = ulNextCluster; 
#else 

[Valery V. Sedletski]

So, it previously worked faster with your improvements, but it got removed? Was it significantly faster? At what degree? 10 times faster on big files or less? Why not try to return this code with some modifications to support parameters in high memory? Why this code is incompatible with OBJ_ANY flag? Maybe, this was because some utilities, like UFAT32.DLL, was 16-bit? Now they are redone to be fully 32-bit, so maybe this does not matter now?

Yes, I remember this issue. It was reported by _dixie_ (who wrote QSINIT) to Ko Myung Hun some years ago -- there was some incompatibilities with buffers located in high memory, but I don't remember the details. So, what prevented the buffers to be located in high memory?

[Valery V. Sedletski]

What if we use pbCluster as a buffer for ReadSector, instead of pBufPosition? Maybe, the problem is with pBufPosition (which is the immediate parameter buffer passed to FS_READ/FS_WRITE) and FSH_DOVOLIO cannot be given a high memory address, so additional memcpy is needed, and this is because the second variant is slower?

[erdmann]

Your understanding is correct.
FSH_DOVOLIO cannot handle data located in high memory and there is no easy way to thunk a 0:32 linear address in high memory to a 16:16 virtual address (the only way would be to use "DosAliasMem" to get an alias in the low memory region. But that is a ring-3 API and I don't think a 16-bit API call exists for this function).
That's why it is indeed necessary to use a "bounce buffer" in low memory. But that means copying data which in turn has a negative impact on the intended increase in processing speed.
That's also the reason why JFS.IFS does not use FSH_DOVOLIO at all. Being an (almost) 32-bit IFS, it uses the 32-bit linear strat3 entry point for all data transfers. But in order to use the strat3 entry point, you'd need to implement FS32_READ and FS32_WRITE ...