On Thursday 18 March 2010 04:50:56 Hugo Etchegoyen wrote:
> Charles,
>
> As I said in a previous mail, I was looking for means of speeding up
> yaffs shutdown during power fail, and that included disabling GC. Your
> last modification (the yaffs_gc_control parameter) addresses that issue.
>
> However, although setting this flag to zero will prevent entering GC, it
> will not abort a currently active GC. I had originally proposed to
> decrease the value of maxCopies in GarbageCollectBlock() to make it
> finer grained, but it looks faster and safer to just check your flag in
> each iteration of the copying loop:
>
>         for (/* init already done */;
>              retVal == YAFFS_OK &&
>              dev->gcChunk < dev->param.nChunksPerBlock &&
>              (bi->blockState == YAFFS_BLOCK_STATE_COLLECTING) &&
>              maxCopies > 0;
>              dev->gcChunk++, oldChunk++) {
>
>             if(dev->param.gcControl &&
>                 (dev->param.gcControl(dev) & 1) == 0)
>                     break;
>
>             if (yaffs_CheckChunkBit(dev, block, dev->gcChunk)) {
>
>                 /* This page is in use and might need to be copied off */
>              ...

Doing that should be fine. It should be OK to interrupt gc at any page. That's 
pretty much what the maxCopy stuff is doing anyway.

I would tend to push the gcControl check into the for().




>
> As yaffs may now be shut down disabling GC, and depending on how much
> data is written with GC disabled (less than a block size in my case),
> maybe at initialization time there might be less free blocks than
> advisable. So I thought that it might be useful to catch up with delayed
> GCs at the end of yaffs_GutsInitialise(). Do you think this is
> necessary/advisable?

I don't think it is necessary.

> Would this code handle that correctly? 
>
>     /* Clean up any aborted checkpoint data */
>     if(!dev->isCheckpointed && dev->blocksInCheckpoint > 0)
>         yaffs_InvalidateCheckpoint(dev);
>


>     /* Honor any pending GCs */
>     do {
>         x = dev->nErasedBlocks;
>         yaffs_CheckGarbageCollection(dev);
>     } while ( dev->gcBlock > 0 || x < dev->nErasedBlocks );
>

There is no need to resume any gc per se. yaffs_CheckGarbageCollection() 
already does iteration.

If you do it here then the checkpoint will be immediately invalidated which is 
perhaps not a good idea. You might as well just leave this to be sorted out 
on the first write.


>     T(YAFFS_TRACE_TRACING,
>       (TSTR("yaffs: yaffs_GutsInitialise() done.\n" TENDSTR)));
>
>     return YAFFS_OK;
>
> I also noticed that there is a possible source of delay when updating
> dirty directories, since once started it will iterate over a list of
> pending updates with the yaffs lock taken. I propose adding another
> flag, yaffs_udd_control ("udd" for Update Dirty Directories) and the
> corresponding callback (dev->uddControl). Then
> yaffs_UpdateDirtyDirectories() can check this flag in each iteration of
> the updating loop:
>
>     while(!ylist_empty(&dev->dirtyDirectories)){
>
>         if(dev->param.uddControl &&
>             (dev->param.uddControl(dev) & 1) == 0)
>             break;
>
>         link = dev->dirtyDirectories.next;
>         ylist_del_init(link);
>         ...

That should be fine to do,
I'll be adding flags like these when I flesh out the background task with 
background garbage collection etc.

> Please advise if these modifications are OK. I have attached the
> modified files (yaffs_guts.h, yaffs_guts.c and yaffs_fs.c). I have
> compiled and tried the module and it seems to work properly, including
> disabling GC and UDD, writing some data, then enabling them again and
> writing some more. I haven't yet simulated a power fail situation when
> one would abort an ongoing GC or UDD, then write some data, sync and
> unmount. I'll report back as soon as I can try that.
>
> Best regards,
> Hugo