Add rebuilding of block allocation bitmap (if marked as invalid in root block)

[t-w]

This was started in consequence of #63, and discussed further in #64.

The new code in the update does what's in the title - it rebuilds the block allocation bitmap when mounting a volume, if the bitmap is marked as "invalid". There is also a new command-line utility, which allows to display and to enforce rebuilding the bitmap.

While this resolves #63, there is still one thing to consider: 2. from #64 (but that is to further improve the reliability).

There are some new tests (in regtests/) that works as follows:

• the test take the images from regtests/Dumps/ makes a copy, rebuilds the bitmap and compares it to the original (here some surprises - some of the images created with old ADFlib seem not valid(!), see exceptions in the script regtests/Test/bitmap_recreate.sh)

This means that any image copied to regtests/Dumps/ will be checked by the test - so anyone can use the test with any adf image. I have done this on many images and the rebuilt bitmap was always identical (with the exceptions mentioned earlier...). It is not really feasible to add to the repo many more dumps for testing...

So - the code seems to work fine. But, of course, more tests are welcomed, if anyone is willing to do so.

[lclevy]

I trust you 😉

[t-w]

Hi @lclevy. Could you have a look and, if ok., mark the review so that we can merge this? (you probably did this already, but I have pushed one more commit here, so...)

[lclevy]

I wonder if mandatory bitmap rebuilding is risky or not, because of custom storage like demo with track loaders. First track look likes amigados, but this is not with a valid filesystem since the operation system is not used. Is it mandatory or not? By default or not?

[lclevy]

Anyway having such code is very good. I just wonder in which cases to activate it or not for end users.. By default, I would not touch the original disk and ask for the fix explicitly, like fsck on Linux

[t-w]

I was going to improve the mounting part later - but you are right, it is better to do it properly from the start. It is good that you pointed this out.

I have prepared some changes which will improve safety of adding this, they can be summarized as follows:

• refactored and updated adfMount
  • after mounting a volume read-only, adfVolReconstructBitmap can be called; this rebuilds the bitmap in memory (no updates on disk); then, it is possible to do adfRemountReadWrite on a volume already mounted as read-only, what will write the reconstructed the bitmap on disk (so this is optional)
  • mounting read-write require that the bitmap on the disk must be valid (otherwise unreliable information is used to allocate blocks...); so if BM_INVALID is set in the root block, the mount fails

So far, I have also made it this way, that for read-only the bitmap is not even read - but here is trouble. Even in read-only mode there are features (adfCountFreeBlocks) that rely on information from the bitmap... So there are 2 choices - either we read the bitmap info from the disk (even if "invalid") or we reconstruct the bitmap (in memory) for read-only mounts, if the bitmap is marked invalid (just without writing it to the disk).

For me it seems that the reconstruction should be done also for read-only (just without writing anything). Note that this of course implies reading all meta-data blocks from the volume (to find all file, directory, link and directory cache blocks used).

Let me know what you think.

[lclevy]

In fact, rebuilding the bitmap is relevant only for real amigados formatted media. Do the user is capable of deciding? Not sure. Like write support, fix metadata must be an explicit and non default behaviour/option. In docs and examples, mounting with write or auto_fix options must have explicit warnings.

[t-w]

Well, I do not quite understand how the bitmap is important only for AmigaOS formatted disks only. The bitmap is used by the ADFlib in both modes:

• read-only: to check free blocks (if a block is free or how many are there, eg. unadf does this)
• read-write: for every block allocation/deallocation

and it does not really matter if it is orig. formatted or not (unless I am missing something).

It would be possible to write the lib in such way so that it would not use the bitmap from the disk (reconstruct everything from other metadata, keep the bitmap only in memory and mark the bitmap as invalid for any future writes...). But I am not sure this is the right way...

If I understand well - AmigaDos was rebuilding the bitmap automatically with DiskValidator (which, if I remember well, had its own set of issues) if the bitmap was marked as invalid. So adding rebuilding the bitmap to the ADFlib is basically reimplementing the feature normally present on the original filesystem. IMHO, not rebuilding the bitmap and using an invalid one, can be more harmful.

I have just pushed a few changes that I think deal with this in a reasonable way - adfMount works as follows:

    if ( adfReadRootBlock ( vol, (uint32_t) vol->rootBlock, &root ) != RC_OK ) {
        adfEnv.eFct ( "adfMount : invalid RootBlock, sector %u", vol->rootBlock );
        vol->mounted = FALSE;
        return NULL;
    }

    RETCODE rc = adfBitmapAllocate ( vol );
    if ( rc != RC_OK ) {
            adfEnv.eFct ( "adfMount : adfBitmapAllocate() returned error %d, "
                          "mounting volume %s failed", rc, vol->volName );
            adfUnMount ( vol );
            return NULL;
    }

    rc = adfReadBitmap ( vol, &root );
    if ( rc != RC_OK ) {
        adfEnv.eFct ( "adfMount : adfReadBitmap() returned error %d, "
                      "mounting volume %s failed", rc, vol->volName );
        adfUnMount ( vol );
        return NULL;
    }

    if ( root.bmFlag != BM_VALID ) {
        if ( vol->readOnly == TRUE ) {
            rc = adfReconstructBitmap ( vol, &root );
            if ( rc != RC_OK ) {
                adfEnv.eFct ( "adfMount : adfReconstructBitmap() returned error %d, "
                              "mounting volume %s failed", rc, vol->volName );
                adfUnMount ( vol );
                return NULL;
            }
        } else {
            adfEnv.eFct ( "adfMount : block allocation bitmap marked invalid in root block, "
                          "mounting the volume %s read-write not possible", vol->volName );
            adfUnMount ( vol );
            return NULL;
        }
    }

in short:

• always reads the bitmap from the volume (mounting fails on error)
• if the bitmap in marked "invalid"
  • for read-only: it reconstructs the bitmap in memory (no overwriting the existing one on the volume)
  • for read-write: mount fails, as block allocation information is not valid (cannot write reliably)

Beyond that, the volume mounted "read-only" can be remounted as read-write, and by this, if the bitmap was reconstructed in memory (was invalid), it is written to the volume. So an update of the bitmap can happen only on an explicit call to adfRemountReadWrite (sure, it has to be documented).

All this trouble is coming with write support... If it is supposed to be reliable, it has to use correct block allocation data.

Other option could be, for instance, always only read the bitmap in whatever mode and let the lib user to execute adfReconstructBitmap himself, if he wants to. The trouble that I see with this is that users also can just mount stuff read-write, write data to it, and then get surprised by the corrupted data due to the use of an invalid bitmap. As normally they can expect reliable work of such functions, I think. They would have to check root block themselves (load it and check state) and decide if they use the bitmap or reconstruct it. Do you think it would be better like this? (power to the library user, but responsibility, too...)

Concerning if the operations are explicit - the read-write mode is selected rather explicitly by a very human readable constant-defined parameters (there was a long discussion about this before...). Concerning bitmap - I see 2 options above, either safe read-write mount and writing data, or power to the user. I am rather for the 1st option (safer, shorter client code), but maybe for flexibility the second is better ...

[t-w]

I have changed the code so that:

• by default it always reads the bitmap from volume (when mounting)
• the validity of the bitmap must be checked in the user code (in principal, if invalid, it should be rebuild to prevent volume corruption and data loss)
• if the bitmap is marked invalid - the lib gives only a warning on mounting
• bitmap reconstruction can be done with adfReconstructBitmap() and is done in-memory, it is not written until other write operations (so as before) or an explicit call to adfUpdateBitmap()

So power, flexibility and responsibility for using an invalid bitmap goes to the client code.

[lclevy]

You've got 'trackdisks', where there is no filesystem apart bootblock. Data is loader via specific code (track loader) inside bootblock.

[lclevy]

https://github.com/deplinenoise/trackloader

[lclevy]

If inserted in an already booted system. Amiga os might detect it as invalid. If filesystem is detected as corrupted, I would block write support.

[t-w]

If there is no valid metadata, ie. no rootblocks, mounting a volume from these "trackdisks" will fail (on reading the rootblock). Basically this blocks write mode for such cases. There is no change on this, it was like this before.

Note that because volume's bitmap was (and is) always loaded (as is the rootblock during mounting), there was (and there is) no way to mount such volume just for accessing raw blocks of a volume. For now, such access has to be done using blocks on the device level.

Support for this could be eventually added, it will require careful redesign on several pieces, so that bitmap is not required on a mounted volume, but, for instance, is used in a lazy way (loaded when needed). This would be like another layer of mounting - without the filesystem (accessing only raw volume blocks) or with the filesystem (accessing filesystem-specific, byte-reordered blocks with filesystem (meta)data etc.).

Or what you mean above is just blocking write access (read-write volume mount) when the bitmap is marked invalid in rootblock? Such blocking is not really necessary, assuming that, as I wrote, responsibility for checking this goes fully to the client code. But, of course, this can be easily put back.

[lclevy]

Looks fine to me.