Re: CVS commit: src/sys

[Jaromír Doleček <jaromir.dolecek%gmail.com@localhost>]

2016-11-11 11:52 GMT+01:00 J. Hannken-Illjes <hannken%eis.cs.tu-bs.de@localhost>:
> Why do we need a TAILQ here, the number of deletions after the first
> element is nearly zero and the list is quite short.

The element would be somewhere towards the end of the list, but likely
not the very last one. Yes, the list is usually short (IIRC the limit
is around 60 per 1MB of log / 1GB of filesystem), so it might not
matter much if we have the error path O(n).

> +wapbl_deallocation_free(struct wapbl *wl, struct wapbl_dealloc *wd,
> +       bool locked)
>
> Better to let the caller always take/release the lock.

I wanted to let the pool_put() be called without the lock, but this
might be not very useful optimisation.

> 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)
> {

I simply want to have it O(1). I think it is useful to keep the error
path there fast, as it would be quite common for wapbl case. Also just
passing the (opaque) pointer makes it simpler.

Thanks for the BAP_ASSIGN() fix. I started investigating only to later
notice you already committed fix.

What you think about removing the NULL initialization, to let compiler
trigger warning there? Seems modern gcc is intelligent enough to
correctly trigger warning, even with the conditional assignment. Of
course it might be not very useful, as hopefully this code won't be
touched again for a long time.

--- ffs_inode.c 11 Nov 2016 10:50:16 -0000      1.123
+++ ffs_inode.c 11 Nov 2016 20:47:04 -0000
@@ -583,8 +583,8 @@ ffs_indirtrunc(struct inode *ip, daddr_t
        int i;
        struct buf *bp;
        struct fs *fs = ip->i_fs;
-       int32_t *bap1 = NULL;
-       int64_t *bap2 = NULL;
+       int32_t *bap1;
+       int64_t *bap2;
        struct vnode *vp;
        daddr_t nb, nlbn, last;
        char *copy = NULL;

Jaromir