Re: ffs_balloc_ufs1 error handling

[Andrew Doran <ad%netbsd.org@localhost>]

On Mon, Dec 08, 2008 at 01:50:25PM +0100, Joerg Sonnenberger wrote:

> On Sun, Dec 07, 2008 at 10:11:14PM +0000, Andrew Doran wrote:
> > On Sun, Dec 07, 2008 at 09:05:20PM +0100, Joerg Sonnenberger wrote:
> > 
> > > from my understand of the code and the history of ffs_balloc_ufs1,
> > > the first if after fail: is meant to flush the vnode content to disk to
> > > break up dependencies. From FreeBSD's history, this is the cheap
> > > approach to handle erroring out for softdep filesystems when they become
> > > full.
> > 
> > 1. It doesn't work for softdep, it hangs.
> 
> Huh? For softdep the check is invariant.
>
> > 2. It does what the comment above it says.
> 
> Assuming you talk about the whole if (unwindidx >= 0) block:
> No, it doesn't. The deallocation of indirect blocks happens in the for
> loop after the if block.
>
> > 3. It also attempts to invalidate blocks associated with the vnode.
> >    It is not clear to me at a glance if that works as intended.
> 
> Same assumption. This was added for the sake of softdep to simplify the
> dependency tracking. It was originally a VOP_FLUSH. I don't see any
> point in doing this for the !softdep cache -- it just forces more sync
> io than necessary.
>
> > Yes, it will corrupt full file systems if they are not using softdep.
> 
> What base do you have for this conclusion? Testing does not reveal any
> corruption. As I said -- until the softdep merge nothing at all was done
> here, just freeing of the indirect blocks.

Sorry but you're wrong on all 4 counts. Please read the code more closely.
With your patch the deallocated indirect blocks are left dangling, producing
a corrupted file system.

Thanks,
Andrew