Re: Bogus KASSERT() in LFS?

[Eduardo Horvath <eeh%NetBSD.org@localhost>]

On Wed, 5 Jan 2011, Martin Husemann wrote:

> On Wed, Jan 05, 2011 at 04:25:09PM +0000, Eduardo Horvath wrote:
> > I think you're right.  While I'm pretty sure that curpg won't be NULL on 
> > the first iteration, I think it can be NULL on subsequent iterations.  I'd 
> > commit that change.
> 
> It shouldn't get there on subsequent iterations if it pulled a NULL out
> of the TAILQ because it explicitily breaks out of the loop in that case.
> 
> Why do you think it can't happen initially?

Looking at the code:

1845:   top:
1846:        by_list = (vp->v_uobj.uo_npages <=
1847:                   ((endoffset - startoffset) >> PAGE_SHIFT) *
1848:                   UVM_PAGE_TREE_PENALTY);
1849:        any_dirty = 0;
1850:
1851:        if (by_list) {
1852:                curpg = TAILQ_FIRST(&vp->v_uobj.memq);

If by_list is set we'll always get here, and I don't think we'd be called 
if the vnode had no pages at all, 'cause lfs_putpages checks for that 
condition and exits immediately if it's true:

2062:         * If there are no pages, don't do anything.
2063:         */
2064:        if (vp->v_uobj.uo_npages == 0) {
2065:                if (TAILQ_EMPTY(&vp->v_uobj.memq) &&
2066:                    (vp->v_iflag & VI_ONWORKLST) &&
2067:                    LIST_FIRST(&vp->v_dirtyblkhd) == NULL) {
2068:                        vp->v_iflag &= ~VI_WRMAPDIRTY;
2069:                        vn_syncer_remove_from_worklist(vp);
2070:                }
2071:                mutex_exit(&vp->v_interlock);
2072:
2073:                /* Remove us from paging queue, if we were on it */
2074:                mutex_enter(&lfs_lock);
2075:                if (ip->i_flags & IN_PAGING) {
2076:                        ip->i_flags &= ~IN_PAGING;
2077:                        TAILQ_REMOVE(&fs->lfs_pchainhd, ip, i_lfs_pchain);
2078:                }
2079:                mutex_exit(&lfs_lock);
2080:                return 0;
2081:        }


Later:

1853:        else {
1854:                soff = startoffset;
1855:        }
1856:        while (by_list || soff < MIN(blkeof, endoffset)) {
1857:                if (by_list) {

So if by_list is true, curpg should not be NULL on the first iteration.  
However....

1858:                        /*
1859:                         * Find the first page in a block.  Skip
1860:                         * blocks outside our area of interest or beyond
1861:                         * the end of file.
1862:                         */
1863:                        KASSERT((curpg->flags & PG_MARKER) == 0);
1864:                        if (pages_per_block > 1) {

At the bottom of the loop:

1972:                if (by_list) {
1973:                        curpg = TAILQ_NEXT(curpg, listq.queue);
1974:                } else {
1975:                        soff += MAX(PAGE_SIZE, fs->lfs_bsize);
1976:                }
1977:        }

If we got to the end of the queue, curpg could be NULL at the next 
iteration.

(Sigh.  One of these days I need to get inspired to give LFS an overhaul.  
Recursion in the kernel is not good, and the code needs to be robust 
enough to recover from a corrupted ifile.  In theory, LFS should be much 
more robust than FFS even with logging, since even if you lose parts of 
the platter you may be able to roll back to older versions of a file....)

Eduardo