From db0c2eb38aa0419f2a01f592c31ea383f50df5a3 Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Tue, 28 Feb 2012 20:53:26 -0800 Subject: [PATCH] hammer2 - Correct memory leak, skeleton for vop_nlink * Correct a memory leak related to dangling inodes that were deleted but also marked modified. The modified status prevented the inode from being destroyed even on umount. * Create a skeleton helper for hammer2_vop_nlink (hardlink creation). (non functional). --- sys/vfs/hammer2/TODO | 16 +++++ sys/vfs/hammer2/hammer2.h | 7 ++- sys/vfs/hammer2/hammer2_chain.c | 20 +++++- sys/vfs/hammer2/hammer2_disk.h | 2 +- sys/vfs/hammer2/hammer2_inode.c | 62 +++++++++++++++++- sys/vfs/hammer2/hammer2_subr.c | 2 +- sys/vfs/hammer2/hammer2_vnops.c | 107 +++++++++++++++++++++++++++----- 7 files changed, 192 insertions(+), 24 deletions(-) diff --git a/sys/vfs/hammer2/TODO b/sys/vfs/hammer2/TODO index 0b978a07ff..4370fa02ab 100644 --- a/sys/vfs/hammer2/TODO +++ b/sys/vfs/hammer2/TODO @@ -7,9 +7,25 @@ recursion checking SUBMODIFIED then locking (must clear before the recursion and might need additional synchronization) +* There is definitely a flush race in the hardlink implementation between + the forwarding entries and the actual (hidden) hardlink inode. + + This will require us to associate a small hard-link-adjust structure + with the chain whenever we create or delete hardlinks, on top of + adjusting the hardlink inode itself. Any actual flush to the media + has to synchronize the correct nlinks value based on whether related + created or deleted hardlinks were also flushed. + * When a directory entry is created and also if an indirect block is created and entries moved into it, the directory seek position can potentially become incorrect during a scan. * When a directory entry is deleted a directory seek position depending on that key can cause readdir to skip entries. + + + OPTIMIZATIONS + +* If a file is unlinked buts its descriptors is left open and used, we + should allow data blocks on-media to be reused since there is no + topology left to point at them. diff --git a/sys/vfs/hammer2/hammer2.h b/sys/vfs/hammer2/hammer2.h index ade2d8f62e..0313357f6d 100644 --- a/sys/vfs/hammer2/hammer2.h +++ b/sys/vfs/hammer2/hammer2.h @@ -280,13 +280,16 @@ void hammer2_inode_free(hammer2_inode_t *ip); void hammer2_inode_ref(hammer2_inode_t *ip); void hammer2_inode_drop(hammer2_inode_t *ip); -int hammer2_create_inode(hammer2_mount_t *hmp, +int hammer2_inode_create(hammer2_mount_t *hmp, struct vattr *vap, struct ucred *cred, hammer2_inode_t *dip, const uint8_t *name, size_t name_len, hammer2_inode_t **nipp); -int hammer2_connect_inode(hammer2_inode_t *dip, hammer2_inode_t *nip, +int hammer2_inode_connect(hammer2_inode_t *dip, hammer2_inode_t *nip, + const uint8_t *name, size_t name_len); + +int hammer2_hardlink_create(hammer2_inode_t *ip, hammer2_inode_t *dip, const uint8_t *name, size_t name_len); /* diff --git a/sys/vfs/hammer2/hammer2_chain.c b/sys/vfs/hammer2/hammer2_chain.c index 3ad3dc184c..8d6258eac8 100644 --- a/sys/vfs/hammer2/hammer2_chain.c +++ b/sys/vfs/hammer2/hammer2_chain.c @@ -176,6 +176,7 @@ hammer2_chain_drop(hammer2_mount_t *hmp, hammer2_chain_t *chain) while (chain) { refs = chain->refs; cpu_ccfence(); + KKASSERT(refs > 0); if (refs == 1) { KKASSERT(chain != &hmp->vchain); parent = chain->parent; @@ -351,6 +352,19 @@ hammer2_chain_modify(hammer2_mount_t *hmp, hammer2_chain_t *chain) return; } + /* + * A deleted inode may still be active but unreachable via sync + * because it has been disconnected from the tree. Do not allow + * deleted inodes to be marked as being modified because this will + * bump the refs and never get resolved by the sync, leaving the + * inode structure allocated after umount. + */ + if ((chain->flags & HAMMER2_CHAIN_DELETED) && + chain->bref.type == HAMMER2_BREF_TYPE_INODE) { + KKASSERT(chain->data != NULL); + return; + } + /* * The MODIFIED bit is not yet set, we must allocate the * copy-on-write block. @@ -1430,6 +1444,9 @@ hammer2_chain_delete(hammer2_mount_t *hmp, hammer2_chain_t *parent, hammer2_blockref_t *base; int count; + if (chain->parent != parent) + panic("hammer2_chain_delete: parent mismatch"); + /* * Mark the parent modified so our base[] pointer remains valid * while we move entries. @@ -1485,6 +1502,7 @@ hammer2_chain_delete(hammer2_mount_t *hmp, hammer2_chain_t *parent, * The MODIFIED bit is cleared but we have to remember the old state * in case this deletion is related to a rename. */ + kprintf("chain_delete %p %d refs=%d\n", chain, chain->flags & HAMMER2_CHAIN_MODIFIED, chain->refs); if (chain->flags & HAMMER2_CHAIN_MODIFIED) { atomic_clear_int(&chain->flags, HAMMER2_CHAIN_MODIFIED); atomic_set_int(&chain->flags, HAMMER2_CHAIN_WAS_MODIFIED); @@ -1653,8 +1671,8 @@ hammer2_chain_flush(hammer2_mount_t *hmp, hammer2_chain_t *chain, * Return information on the new block to the parent. */ *parent_bref = chain->bref; - hammer2_chain_drop(hmp, chain); /* drop ref tracking mod bit */ atomic_clear_int(&chain->flags, HAMMER2_CHAIN_MODIFIED); + hammer2_chain_drop(hmp, chain); /* drop ref tracking mod bit */ } else { hammer2_blockref_t *bref; diff --git a/sys/vfs/hammer2/hammer2_disk.h b/sys/vfs/hammer2/hammer2_disk.h index d08ebc954a..52e9b58e8c 100644 --- a/sys/vfs/hammer2/hammer2_disk.h +++ b/sys/vfs/hammer2/hammer2_disk.h @@ -427,7 +427,7 @@ typedef struct hammer2_inode_data hammer2_inode_data_t; #define HAMMER2_OBJTYPE_CDEV 5 #define HAMMER2_OBJTYPE_BDEV 6 #define HAMMER2_OBJTYPE_SOFTLINK 7 -#define HAMMER2_OBJTYPE_HARDLINK 8 +#define HAMMER2_OBJTYPE_HARDLINK 8 /* dummy entry for hardlink */ #define HAMMER2_OBJTYPE_SOCKET 9 #define HAMMER2_OBJTYPE_WHITEOUT 10 diff --git a/sys/vfs/hammer2/hammer2_inode.c b/sys/vfs/hammer2/hammer2_inode.c index 8a62984540..6e415411f7 100644 --- a/sys/vfs/hammer2/hammer2_inode.c +++ b/sys/vfs/hammer2/hammer2_inode.c @@ -196,7 +196,7 @@ hammer2_igetv(hammer2_inode_t *ip, int *errorp) * *nipp, otherwise an error is returned and *nipp is set to NULL. */ int -hammer2_create_inode(hammer2_mount_t *hmp, +hammer2_inode_create(hammer2_mount_t *hmp, struct vattr *vap, struct ucred *cred, hammer2_inode_t *dip, const uint8_t *name, size_t name_len, @@ -286,7 +286,7 @@ hammer2_create_inode(hammer2_mount_t *hmp, * (ip) must be locked. */ int -hammer2_connect_inode(hammer2_inode_t *dip, hammer2_inode_t *ip, +hammer2_inode_connect(hammer2_inode_t *dip, hammer2_inode_t *ip, const uint8_t *name, size_t name_len) { hammer2_mount_t *hmp = dip->hmp; @@ -356,3 +356,61 @@ hammer2_connect_inode(hammer2_inode_t *dip, hammer2_inode_t *ip, return (0); } + +/* + * Create a hardlink forwarding entry (dip, name) to the specified (ip). + * + * This is one of the more complex implementations in HAMMER2. The + * filesystem strictly updates its chains bottom-up in a copy-on-write + * fashion. This makes hardlinks difficult to implement but we've come up + * with a dandy solution. + * + * When a file has more than one link the actual inode is created as a + * hidden directory entry (indexed by inode number) in a common parent of + * all hardlinks which reference the file. The hardlinks in each directory + * are merely forwarding entries to the hidden inode. + * + * Implementation: + * + * Most VOPs can be blissfully unaware of the forwarding entries. + * nresolve, nlink, and remove code have to be forwarding-aware + * in order to return the (ip/vp) for the actual file (and otherwise do + * the right thing). + * + * (1) If the ip we are linking to is a normal embedded inode (nlinks==1) + * we have to replace the directory entry with a forwarding inode + * and move the normal ip/vp to a hidden entry indexed by the inode + * number in a common parent directory. + * + * (2) If the ip we are linking to is already a hidden entry but is not + * a common parent we have to move its entry to a common parent by + * moving the entry upward. + * + * (3) The trivial case is the entry is already hidden and already a + * common parent. We adjust nlinks for the entry and are done. + * (this is the fall-through case). + */ +int +hammer2_hardlink_create(hammer2_inode_t *ip, hammer2_inode_t *dip, + const uint8_t *name, size_t name_len) +{ + return ENOTSUP; +#if 0 + hammer2_inode_t *nip; + hammer2_inode_t *xip; + + + hammer2_inode_t *nip; /* hardlink forwarding inode */ + error = hammer2_inode_create(hmp, NULL, ap->a_cred, + dip, name, name_len, &nip); + if (error) { + KKASSERT(nip == NULL); + return error; + } + KKASSERT(nip->ip_data.type == HAMMER2_OBJTYPE_HARDLINK); + hammer2_chain_modify(&nip->chain); + nip->ip_data.inum = ip->ip_data.inum; + hammer2_chain_put(hmp, &nip->chain); + / +#endif +} diff --git a/sys/vfs/hammer2/hammer2_subr.c b/sys/vfs/hammer2/hammer2_subr.c index f50f270547..8ec83cb7e8 100644 --- a/sys/vfs/hammer2/hammer2_subr.c +++ b/sys/vfs/hammer2/hammer2_subr.c @@ -150,7 +150,7 @@ hammer2_get_dtype(hammer2_inode_t *ip) return (DT_BLK); case HAMMER2_OBJTYPE_SOFTLINK: return (DT_LNK); - case HAMMER2_OBJTYPE_HARDLINK: /* XXX */ + case HAMMER2_OBJTYPE_HARDLINK: /* (never directly associated w/vp) */ return (DT_UNKNOWN); case HAMMER2_OBJTYPE_SOCKET: return (DT_SOCK); diff --git a/sys/vfs/hammer2/hammer2_vnops.c b/sys/vfs/hammer2/hammer2_vnops.c index e3be4ac5fe..235c678b8f 100644 --- a/sys/vfs/hammer2/hammer2_vnops.c +++ b/sys/vfs/hammer2/hammer2_vnops.c @@ -57,7 +57,7 @@ static void hammer2_extend_file(hammer2_inode_t *ip, hammer2_key_t nsize, static void hammer2_truncate_file(hammer2_inode_t *ip, hammer2_key_t nsize); static int hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, - int isdir); + int isdir, int adjlinks); /* * Last reference to a vnode is going away but it is still cached. @@ -820,6 +820,7 @@ hammer2_truncate_file(hammer2_inode_t *ip, hammer2_key_t nsize) if (chain->bref.type == HAMMER2_BREF_TYPE_DATA) { hammer2_chain_delete(hmp, parent, chain); } + /* XXX check parent if empty indirect block & delete */ chain = hammer2_chain_next(hmp, &parent, chain, psize, (hammer2_key_t)-1, HAMMER2_LOOKUP_NOLOCK); @@ -966,7 +967,7 @@ hammer2_vop_nmkdir(struct vop_nmkdir_args *ap) name = ncp->nc_name; name_len = ncp->nc_nlen; - error = hammer2_create_inode(hmp, ap->a_vap, ap->a_cred, + error = hammer2_inode_create(hmp, ap->a_vap, ap->a_cred, dip, name, name_len, &nip); if (error) { KKASSERT(nip == NULL); @@ -1055,9 +1056,7 @@ hammer2_vop_open(struct vop_open_args *ap) } /* - * hammer_vop_advlock { vp, id, op, fl, flags } - * - * MPSAFE - does not require fs_token + * hammer2_vop_advlock { vp, id, op, fl, flags } */ static int @@ -1077,7 +1076,43 @@ hammer2_vop_close(struct vop_close_args *ap) } /* - * hammer_vop_ncreate { nch, dvp, vpp, cred, vap } + * hammer2_vop_nlink { nch, dvp, vp, cred } + * + * Create a hardlink to vp. + */ +static +int +hammer2_vop_nlink(struct vop_nlink_args *ap) +{ + hammer2_inode_t *dip; + hammer2_inode_t *ip; /* inode we are hardlinking to */ + hammer2_mount_t *hmp; + struct namecache *ncp; + const uint8_t *name; + size_t name_len; + int error; + + dip = VTOI(ap->a_dvp); + hmp = dip->hmp; + if (hmp->ronly) + return (EROFS); + + ip = VTOI(ap->a_vp); + + ncp = ap->a_nch->ncp; + name = ncp->nc_name; + name_len = ncp->nc_nlen; + + error = hammer2_hardlink_create(ip, dip, name, name_len); + if (error == 0) { + cache_setunresolved(ap->a_nch); + cache_setvp(ap->a_nch, ap->a_vp); + } + return error; +} + +/* + * hammer2_vop_ncreate { nch, dvp, vpp, cred, vap } * * The operating system has already ensured that the directory entry * does not exist and done all appropriate namespace locking. @@ -1103,7 +1138,7 @@ hammer2_vop_ncreate(struct vop_ncreate_args *ap) name = ncp->nc_name; name_len = ncp->nc_nlen; - error = hammer2_create_inode(hmp, ap->a_vap, ap->a_cred, + error = hammer2_inode_create(hmp, ap->a_vap, ap->a_cred, dip, name, name_len, &nip); if (error) { KKASSERT(nip == NULL); @@ -1146,7 +1181,7 @@ hammer2_vop_nsymlink(struct vop_nsymlink_args *ap) ap->a_vap->va_type = VLNK; /* enforce type */ - error = hammer2_create_inode(hmp, ap->a_vap, ap->a_cred, + error = hammer2_inode_create(hmp, ap->a_vap, ap->a_cred, dip, name, name_len, &nip); if (error) { KKASSERT(nip == NULL); @@ -1222,7 +1257,7 @@ hammer2_vop_nremove(struct vop_nremove_args *ap) name = ncp->nc_name; name_len = ncp->nc_nlen; - error = hammer2_unlink_file(dip, name, name_len, 0); + error = hammer2_unlink_file(dip, name, name_len, 0, 1); if (error == 0) { cache_setunresolved(ap->a_nch); @@ -1254,7 +1289,7 @@ hammer2_vop_nrmdir(struct vop_nrmdir_args *ap) name = ncp->nc_name; name_len = ncp->nc_nlen; - error = hammer2_unlink_file(dip, name, name_len, 1); + error = hammer2_unlink_file(dip, name, name_len, 1, 1); if (error == 0) { cache_setunresolved(ap->a_nch); @@ -1314,16 +1349,20 @@ hammer2_vop_nrename(struct vop_nrename_args *ap) /* * Remove target if it exists */ - error = hammer2_unlink_file(tdip, tname, tname_len, -1); + error = hammer2_unlink_file(tdip, tname, tname_len, -1, 1); if (error && error != ENOENT) goto done; cache_setunresolved(ap->a_tnch); cache_setvp(ap->a_tnch, NULL); /* - * Disconnect ip from the source directory. + * Disconnect ip from the source directory, do not adjust + * the link count. Note that rename doesn't need to understand + * whether this is a hardlink or not, we can just rename the + * forwarding entry and don't even have to adjust the related + * hardlink's link count. */ - error = hammer2_unlink_file(fdip, fname, fname_len, -1); + error = hammer2_unlink_file(fdip, fname, fname_len, -1, 0); if (error) goto done; @@ -1333,7 +1372,7 @@ hammer2_vop_nrename(struct vop_nrename_args *ap) /* * Reconnect ip to target directory. */ - error = hammer2_connect_inode(tdip, ip, tname, tname_len); + error = hammer2_inode_connect(tdip, ip, tname, tname_len); if (error == 0) { cache_rename(ap->a_fnch, ap->a_tnch); @@ -1348,11 +1387,22 @@ done: /* * Unlink the file from the specified directory inode. The directory inode * does not need to be locked. + * + * isdir determines whether a directory/non-directory check should be made. + * No check is made if isdir is set to -1. + * + * adjlinks tells unlink that we want to adjust the nlinks count of the + * inode. When removing the last link for a NON forwarding entry we can + * just ignore the link count... no point updating the inode that we are + * about to dereference, it would just result in a lot of wasted I/O. + * + * However, if the entry is a forwarding entry (aka a hardlink), and adjlinks + * is non-zero, we have to locate the hardlink and adjust its nlinks field. */ static int hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, - int isdir) + int isdir, int adjlinks) { hammer2_mount_t *hmp; hammer2_chain_t *parent; @@ -1360,11 +1410,9 @@ hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, hammer2_chain_t *dparent; hammer2_chain_t *dchain; hammer2_key_t lhc; - struct vnode *vp; int error; error = 0; - /*vap->va_nlink = ip->ip_data.nlinks;*/ hmp = dip->hmp; lhc = hammer2_dirhash(name, name_len); @@ -1432,6 +1480,23 @@ hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, /* dchain NULL */ } +#if 0 + /* + * If adjlinks is non-zero this is a real deletion (otherwise it is + * probably a rename). XXX + */ + if (adjlinks) { + if (chain->data->ipdata.type == HAMMER2_OBJTYPE_HARDLINK) { + /*hammer2_adjust_hardlink(chain->u.ip, -1);*/ + /* error handling */ + } else { + waslastlink = 1; + } + } else { + waslastlink = 0; + } +#endif + /* * Found, the chain represents the inode. Remove the parent reference * to the chain. The chain itself is no longer referenced and will @@ -1442,7 +1507,13 @@ hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, /* XXX nlinks (hardlink special case) */ /* XXX nlinks (parent directory) */ +#if 0 + /* + * Destroy any associated vnode, but only if this was the last + * link. XXX this might not be needed. + */ if (chain->u.ip->vp) { + struct vnode *vp; vp = hammer2_igetv(chain->u.ip, &error); if (error == 0) { vn_unlock(vp); @@ -1451,6 +1522,7 @@ hammer2_unlink_file(hammer2_inode_t *dip, const uint8_t *name, size_t name_len, vrele(vp); } } +#endif error = 0; done: @@ -1697,6 +1769,7 @@ struct vop_ops hammer2_vnode_vops = { .vop_access = hammer2_vop_access, .vop_advlock = hammer2_vop_advlock, .vop_close = hammer2_vop_close, + .vop_nlink = hammer2_vop_nlink, .vop_ncreate = hammer2_vop_ncreate, .vop_nsymlink = hammer2_vop_nsymlink, .vop_nremove = hammer2_vop_nremove, -- 2.41.0