From d5fabb7006ddb6d88f786e86892f6f5381b584ea Mon Sep 17 00:00:00 2001 From: Matthew Dillon Date: Sun, 28 Apr 2013 16:24:56 -0700 Subject: [PATCH] hammer2 - Major restructuring, part 2/several * Stabilization pass on hammer2_inode_connect() and hammer2_hardlink_consolidate(). Adjust the API to accomodate requirements (primarily that the caller is responsible for holding the inode locked and for replacing ip->chain). * Add hammer2_inode_repoint() to handle the guts of replacing ip->chain. * Flush code shouldn't be messing the chain->bref when deleting the chain as the chain may be used in a duplication operation after the call. * basic rm/mv/ln operations now work (note: we still need to code the reparenting of sub-chains based on chain->duplink, and hardlinks are still buggy when parent directories get renamed). --- sys/vfs/hammer2/hammer2.h | 2 +- sys/vfs/hammer2/hammer2_flush.c | 13 ++-- sys/vfs/hammer2/hammer2_inode.c | 102 ++++++++++++++++++++++---------- sys/vfs/hammer2/hammer2_subr.c | 3 + sys/vfs/hammer2/hammer2_vnops.c | 28 ++++----- 5 files changed, 96 insertions(+), 52 deletions(-) diff --git a/sys/vfs/hammer2/hammer2.h b/sys/vfs/hammer2/hammer2.h index 86ea530778..fed1ad697d 100644 --- a/sys/vfs/hammer2/hammer2.h +++ b/sys/vfs/hammer2/hammer2.h @@ -436,6 +436,7 @@ void hammer2_inode_put(hammer2_inode_t *ip); void hammer2_inode_free(hammer2_inode_t *ip); void hammer2_inode_ref(hammer2_inode_t *ip); void hammer2_inode_drop(hammer2_inode_t *ip); +void hammer2_inode_repoint(hammer2_inode_t *ip, hammer2_chain_t **chainp); int hammer2_inode_calc_alloc(hammer2_key_t filesize); hammer2_inode_t *hammer2_inode_create(hammer2_trans_t *trans, @@ -447,7 +448,6 @@ hammer2_chain_t *hammer2_inode_duplicate(hammer2_trans_t *trans, hammer2_chain_t *ochain, hammer2_inode_t *dip, int *errorp); int hammer2_inode_connect(hammer2_trans_t *trans, - hammer2_inode_t *ip, hammer2_inode_t *dip, hammer2_chain_t **chainp, const uint8_t *name, size_t name_len); diff --git a/sys/vfs/hammer2/hammer2_flush.c b/sys/vfs/hammer2/hammer2_flush.c index ea750a117c..83bec77f87 100644 --- a/sys/vfs/hammer2/hammer2_flush.c +++ b/sys/vfs/hammer2/hammer2_flush.c @@ -782,6 +782,14 @@ hammer2_chain_flush_scan2(hammer2_chain_t *child, void *data) * blockref updates do not touch modify_tid. Instead, mirroring * operations always reconcile the entire array during their * mirror_tid based recursion. + * + * WARNING! Deleted chains may still be used by the filesystem + * in a later duplication, for example in a rename() + * operation. Also any topological movement of the + * related blocks. + * + * We adjust the parent's bref pointer to the child but + * we do not modify the contents of the child. */ if (action == HC_DELETE) { if (base) { @@ -805,11 +813,6 @@ hammer2_chain_flush_scan2(hammer2_chain_t *child, void *data) } cleanup: - /* - * Deletions should also zero-out the child's bref for safety. - */ - if (action == HC_DELETE) - bzero(&child->bref, sizeof(child->bref)); /* * Cleanup the child's MOVED flag and unlock the child. diff --git a/sys/vfs/hammer2/hammer2_inode.c b/sys/vfs/hammer2/hammer2_inode.c index feff9e6bf9..3266ba61dc 100644 --- a/sys/vfs/hammer2/hammer2_inode.c +++ b/sys/vfs/hammer2/hammer2_inode.c @@ -584,6 +584,9 @@ retry: * Connect the target inode to the media topology at (dip, name, len). * This function creates a directory entry and replace (*chainp). * + * The caller usually holds the related inode exclusive locked through this + * call and is also responsible for replacing ip->chain after we return. + * * If (*chainp) was marked DELETED then it represents a terminus inode * with no other nlinks, we can simply duplicate the chain (in-memory * chain structures cannot be moved within the in-memory topology, only @@ -602,14 +605,15 @@ retry: * might reside in some parent directory. It will not be the * OBJTYPE_HARDLINK pointer. * - * WARNING! This function will also replace ip->chain. The related inode - * must be locked exclusively or would wind up racing other - * modifying operations on the same inode which then wind up - * modifying under the old chain instead of the new chain. + * WARNING! The caller is likely holding ip/ip->chain locked exclusively. + * Replacing ip->chain here would create confusion so we leave + * it to the caller to do that. + * + * (The caller is expected to hold the related inode exclusively) */ int hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip, - hammer2_inode_t *ip, hammer2_chain_t **chainp, + hammer2_chain_t **chainp, const uint8_t *name, size_t name_len) { hammer2_inode_data_t *ipdata; @@ -633,7 +637,7 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip, parent = hammer2_chain_lookup_init(dip->chain, 0); lhc = hammer2_dirhash(name, name_len); - hlink = ((ochain->flags & HAMMER2_CHAIN_DELETED) != 0); + hlink = ((ochain->flags & HAMMER2_CHAIN_DELETED) == 0); kprintf("reconnect hlink=%d name=%*.*s\n", hlink, (int)name_len, (int)name_len, name); @@ -703,13 +707,10 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip, parent = NULL; /* - * ochain still active. - * - * Handle the error case + * nchain should be NULL on error, leave ochain (== *chainp) alone. */ if (error) { KKASSERT(nchain == NULL); - hammer2_chain_unlock(ochain); return (error); } @@ -726,6 +727,8 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip, /* * Create the HARDLINK pointer. oip represents the hardlink * target in this situation. + * + * We will return ochain (the hardlink target). */ hammer2_chain_modify(trans, nchain, 0); KKASSERT(name_len < HAMMER2_INODE_MAXNAME); @@ -779,21 +782,57 @@ hammer2_inode_connect(hammer2_trans_t *trans, hammer2_inode_t *dip, } ipdata->nlinks = 1; } + + /* + * We are replacing ochain with nchain, unlock ochain. In the + * case where ochain is left unchanged the code above sets + * nchain to ochain and ochain to NULL, resulting in a NOP here. + */ if (ochain) hammer2_chain_unlock(ochain); *chainp = nchain; + return (0); +} + +/* + * Caller must hold exactly ONE exclusive lock on the inode. *nchainp + * must be exclusive locked (its own exclusive lock even if it is the + * same as ip->chain). + * + * This function replaces ip->chain. The exclusive lock on the passed + * nchain is inherited by the inode and the caller becomes responsible + * for unlocking it when the caller unlocks the inode. + * + * ochain was locked by the caller indirectly via the inode lock. Since + * ip->chain is being repointed, we become responsible for cleaning up + * that lock. + * + * Return *nchainp = NULL as a safety. + */ +void +hammer2_inode_repoint(hammer2_inode_t *ip, hammer2_chain_t **nchainp) +{ + hammer2_chain_t *nchain = *nchainp; + hammer2_chain_t *ochain; + /* - * Replace ip->chain if necessary. XXX inode sub-topology replacement. + * Repoint ip->chain if necessary. + * + * (Inode must be locked exclusively by parent) */ - if (ip->chain != nchain) { - hammer2_chain_ref(nchain); /* ip->chain */ - if (ip->chain) - hammer2_chain_drop(ip->chain); /* ip->chain */ + ochain = ip->chain; + if (ochain != nchain) { + hammer2_chain_ref(nchain); /* for ip->chain */ ip->chain = nchain; + if (ochain) { + hammer2_chain_unlock(ochain); + hammer2_chain_drop(ochain); /* for ip->chain */ + } + } else { + hammer2_chain_unlock(nchain); } - - return (0); + *nchainp = NULL; } /* @@ -1019,11 +1058,17 @@ hammer2_inode_calc_alloc(hammer2_key_t filesize) } /* - * Given an unlocked ip consolidate for hardlink creation, adding (nlinks) - * to the file's link count and potentially relocating the file to a - * directory common to ip->pip and tdip. + * Given an exclusively locked inode we consolidate its chain for hardlink + * creation, adding (nlinks) to the file's link count and potentially + * relocating the file to a directory common to ip->pip and tdip. + * + * Returns a locked chain in (*chainp) (the chain's lock is in addition to + * any lock it might already have due to the inode being locked). *chainp + * is set unconditionally and its previous contents can be garbage. * - * If the file has to be relocated ip->chain will also be adjusted. + * The caller is responsible for replacing ip->chain, not us. For certain + * operations such as renames the caller may do additional manipulation + * of the chain before replacing ip->chain. */ int hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip, @@ -1043,7 +1088,6 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip, * Extra lock on chain so it can be returned locked. */ hmp = tdip->hmp; - hammer2_inode_lock_ex(ip); chain = ip->chain; error = hammer2_chain_lock(chain, HAMMER2_RESOLVE_ALWAYS); @@ -1051,18 +1095,15 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip, if (nlinks == 0 && /* no hardlink needed */ (chain->data->ipdata.name_key & HAMMER2_DIRHASH_VISIBLE)) { - hammer2_inode_unlock_ex(ip); *chainp = chain; return (0); } if (hammer2_hardlink_enable < 0) { /* fake hardlinks */ - hammer2_inode_unlock_ex(ip); *chainp = chain; return (0); } if (hammer2_hardlink_enable == 0) { /* disallow hardlinks */ - hammer2_inode_unlock_ex(ip); hammer2_chain_unlock(chain); *chainp = NULL; return (ENOTSUP); @@ -1166,16 +1207,14 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip, } /* - * Replace ip->chain with nchain (ip is still locked). + * Return the new chain. */ - hammer2_chain_ref(nchain); /* ip->chain */ - if (ip->chain) - hammer2_chain_drop(ip->chain); /* ip->chain */ - ip->chain = nchain; - hammer2_chain_unlock(chain); *chainp = nchain; } else { + /* + * Return an error + */ hammer2_chain_unlock(chain); *chainp = NULL; } @@ -1184,7 +1223,6 @@ hammer2_hardlink_consolidate(hammer2_trans_t *trans, hammer2_inode_t *ip, * Cleanup, chain/nchain already dealt with. */ done: - hammer2_inode_unlock_ex(ip); hammer2_inode_drop(cdip); return (error); diff --git a/sys/vfs/hammer2/hammer2_subr.c b/sys/vfs/hammer2/hammer2_subr.c index c3c9cd3acf..a725389d36 100644 --- a/sys/vfs/hammer2/hammer2_subr.c +++ b/sys/vfs/hammer2/hammer2_subr.c @@ -55,6 +55,9 @@ * * NOTE: We don't combine the inode/chain lock because putting away an * inode would otherwise confuse multiple lock holders of the inode. + * + * WARNING! hammer2_inode_repoint() expects exactly one exclusive lock + * on ip->chain. */ void hammer2_inode_lock_ex(hammer2_inode_t *ip) diff --git a/sys/vfs/hammer2/hammer2_vnops.c b/sys/vfs/hammer2/hammer2_vnops.c index cc1d6e1405..76796b0f22 100644 --- a/sys/vfs/hammer2/hammer2_vnops.c +++ b/sys/vfs/hammer2/hammer2_vnops.c @@ -1765,7 +1765,7 @@ hammer2_vop_nlink(struct vop_nlink_args *ap) * returned chain is locked. */ ip = VTOI(ap->a_vp); - hammer2_inode_ref(ip); + hammer2_inode_lock_ex(ip); error = hammer2_hardlink_consolidate(&trans, ip, &chain, dip, 1); if (error) goto done; @@ -1774,17 +1774,18 @@ hammer2_vop_nlink(struct vop_nlink_args *ap) * Create a directory entry connected to the specified chain. * This function unlocks and NULL's chain on return. */ - error = hammer2_inode_connect(&trans, dip, ip, &chain, name, name_len); - if (chain) { - hammer2_chain_unlock(chain); - chain = NULL; - } + error = hammer2_inode_connect(&trans, dip, &chain, name, name_len); if (error == 0) { + hammer2_inode_repoint(ip, &chain); cache_setunresolved(ap->a_nch); cache_setvp(ap->a_nch, ap->a_vp); } done: - hammer2_inode_drop(ip); + if (chain) { + hammer2_chain_unlock(chain); + chain = NULL; + } + hammer2_inode_unlock_ex(ip); hammer2_trans_done(&trans); return error; @@ -2031,7 +2032,7 @@ hammer2_vop_nrename(struct vop_nrename_args *ap) hammer2_trans_init(&trans, hmp); /* - * ip is the inode being removed. If this is a hardlink then + * ip is the inode being renamed. If this is a hardlink then * ip represents the actual file and not the hardlink marker. */ ip = VTOI(fncp->nc_vp); @@ -2066,6 +2067,7 @@ hammer2_vop_nrename(struct vop_nrename_args *ap) * * The returned chain will be locked. */ + hammer2_inode_lock_ex(ip); error = hammer2_hardlink_consolidate(&trans, ip, &chain, tdip, 0); if (error) goto done; @@ -2094,18 +2096,16 @@ hammer2_vop_nrename(struct vop_nrename_args *ap) * op (that might have to lock a vnode). */ error = hammer2_inode_connect(&trans, tdip, - ip, &chain, - tname, tname_len); + &chain, tname, tname_len); if (error == 0) { - if (chain) { - hammer2_chain_unlock(chain); - chain = NULL; - } + KKASSERT(chain != NULL); + hammer2_inode_repoint(ip, &chain); cache_rename(ap->a_fnch, ap->a_tnch); } done: if (chain) hammer2_chain_unlock(chain); + hammer2_inode_unlock_ex(ip); hammer2_inode_drop(ip); hammer2_trans_done(&trans); -- 2.41.0