Re: CVS commit: src/sys

["J. Hannken-Illjes" <hannken%eis.cs.tu-bs.de@localhost>]

> On 10 Nov 2016, at 21:56, Jaromir Dolecek <jdolecek%netbsd.org@localhost> wrote:
> 
> Module Name:	src
> Committed By:	jdolecek
> Date:		Thu Nov 10 20:56:32 UTC 2016
> 
> Modified Files:
> 	src/sys/kern: vfs_wapbl.c
> 	src/sys/sys: wapbl.h
> 	src/sys/ufs/ffs: ffs_inode.c ffs_wapbl.c
> 	src/sys/ufs/ufs: ufs_wapbl.h
> 
> Log Message:
> during truncate with wapbl, register deallocation for upper indirect block
> before recursing into lower blocks, to make sure that it will be removed after
> all its referenced blocks are removed
> 
> fixes 'ffs_blkfree_common: freeing free block' panic triggered by
> ufs_truncate_retry() when just the upper indirect block registration failed,
> code tried to free the lower blocks again after wapbl flush
> 
> problem found by hannken@, thank you
> 
> 
> To generate a diff of this commit:
> cvs rdiff -u -r1.85 -r1.86 src/sys/kern/vfs_wapbl.c

-       SIMPLEQ_HEAD(, wapbl_dealloc) wl_dealloclist;   /* lm:  list head */
+       TAILQ_HEAD(, wapbl_dealloc) wl_dealloclist;     /* lm:  list head */

This should have been committed on its own.  It makes reading the diff
quite difficult.

Why do we need a TAILQ here, the number of deletions after the first
element is nearly zero and the list is quite short.


+wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd,
+       bool locked)

Better to let the caller always take/release the lock.


+wapbl_register_deallocation(struct wapbl *wl, daddr_t blk, int len, bool force,
+    void **cookiep)

+       if (cookiep)
+               *cookiep = wd;

+wapbl_unregister_deallocation(struct wapbl *wl, void *cookie)

Returning a pointer to an arbitrary list element and using it
later is bad design.  Would be better to define as:

wapbl_unregister_deallocation(struct wapbl *wl, daddr_t blk, int len)
{
	struct wapbl_dealloc *wd;

	mutex_enter(&wl->wl_mtx);
	TAILQ_FOREACH(wd, &wl->wl_dealloclist, wd_entries) {
		if (wd->wd_blkno == blk && wd->wd_len == len)
			break;
	}
	KASSERT(wd != NULL);
	wapbl_deallocation_free(wl, wd);
	mutex_exit(&wl->wl_mtx);
}

> cvs rdiff -u -r1.19 -r1.20 src/sys/sys/wapbl.h
> cvs rdiff -u -r1.121 -r1.122 src/sys/ufs/ffs/ffs_inode.c
> cvs rdiff -u -r1.35 -r1.36 src/sys/ufs/ffs/ffs_wapbl.c
> cvs rdiff -u -r1.12 -r1.13 src/sys/ufs/ufs/ufs_wapbl.h

--
J. Hannken-Illjes - hannken%eis.cs.tu-bs.de@localhost - TU Braunschweig (Germany)