few ZAP methods to access by dnode #5464

bzzz77 · 2016-12-09T08:35:22Z

add few ZAP methods to access by dnode and save on dnode#->dnode_t lookup
dmu_tx_add_new_object() to use dnode and do not translate dnode#

Authored by: Alexey Zhuralev alexey@udm.ru

mention-bot · 2016-12-09T08:35:24Z

@bzzz77, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @ahrens and @nedbass to be potential reviewers.

ahrens

This looks great, only style feedback from me. Note that the cstyle tool may be able to catch at least some of these.

ahrens · 2016-12-09T22:06:18Z

module/zfs/dmu_tx.c

+		}
+	}
+	txh = dmu_tx_hold_dnode_impl(tx, os, dn, type, arg1, arg2);
+	if (txh && txh->txh_dnode)


On illumos, we try to explicitly convert things to boolean, e.g. txh != NULL && txh->txh_dnode != NULL here. Not sure if you do that for ZoL but I find it cleaner.

ahrens · 2016-12-09T22:06:40Z

module/zfs/dmu_tx.c

-		(void) dmu_tx_hold_object_impl(tx, os,
-		    object, THT_NEWOBJECT, 0, 0);
+		(void) dmu_tx_hold_dnode_impl(tx, os,
+				dn, THT_NEWOBJECT, 0, 0);


indentation's wrong here, should be prev line + 4 spaces

ahrens · 2016-12-09T22:07:06Z

module/zfs/dmu_tx.c

+	txh = dmu_tx_hold_dnode_impl(tx, os, dn, type, arg1, arg2);
+	if (txh && txh->txh_dnode)
+		dnode_rele(txh->txh_dnode, tx);
+	return txh;


put parenthesis around return value (txh)

ahrens · 2016-12-09T22:09:00Z

module/zfs/zap_micro.c

@@ -1168,6 +1162,38 @@ zap_add(objset_t *os, uint64_t zapobj, const char *key,
 	}
 	ASSERT(zap == zn->zn_zap);
 	zap_name_free(zn);
+	return err;


ahrens · 2016-12-09T22:09:20Z

module/zfs/zap_micro.c

+	int err;
+
+	err = zap_lockdir(os, zapobj, tx, RW_WRITER, TRUE, TRUE, FTAG, &zap);
+	if (err)


ahrens · 2016-12-09T22:09:39Z

module/zfs/zap_micro.c

+	int err;
+
+	err = zap_lockdir_by_dnode(dn, tx, RW_WRITER, TRUE, TRUE, FTAG, &zap);
+	if (err)


ahrens · 2016-12-09T22:09:53Z

module/zfs/zap_micro.c

@@ -1288,23 +1314,16 @@ zap_remove(objset_t *os, uint64_t zapobj, const char *name, dmu_tx_t *tx)
 	return (zap_remove_norm(os, zapobj, name, MT_EXACT, tx));
 }

-int
-zap_remove_norm(objset_t *os, uint64_t zapobj, const char *name,
+int zap_remove_impl(zap_t *zap, const char *name,


add newline after return type

behlendorf · 2016-12-09T22:51:43Z

Here's a link to the style warnings from the buildbot.

jxiong · 2016-12-09T23:00:36Z

@bzzz77 make cstyle will be helpful to find style issues.

behlendorf · 2016-12-09T23:16:06Z

The ztest failures are new and definitely look related to this patch.

* Backtrace 
*
#0  0x00007f2f259555f7 in raise () from /lib64/libc.so.6
#1  0x00007f2f25956ce8 in abort () from /lib64/libc.so.6
#2  0x00007f2f26eed357 in vpanic (fmt=0x7f2f2709ac40 "No such hold %p on refcount %llx", adx=adx@entry=0x7f2ee33cd560) at kernel.c:949
#3  0x00007f2f26eed409 in panic (fmt=fmt@entry=0x7f2f2709ac40 "No such hold %p on refcount %llx") at kernel.c:958
#4  0x00007f2f26f83784 in refcount_remove_many (rc=rc@entry=0x286a108, number=number@entry=1, holder=holder@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/refcount.c:195
#5  0x00007f2f26f83915 in refcount_remove (rc=rc@entry=0x286a108, holder=holder@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/refcount.c:203
#6  0x00007f2f26f199f7 in dbuf_rele_and_unlock (db=0x286a070, tag=tag@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/dbuf.c:2935
#7  0x00007f2f26f1a1e8 in dbuf_rele (db=<optimized out>, tag=tag@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/dbuf.c:2909
#8  0x00007f2f26f2096d in dmu_buf_rele (db=<optimized out>, tag=tag@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/dbuf.c:2915
#9  0x00007f2f27029979 in zap_unlockdir (zap=zap@entry=0x28c3440, tag=tag@entry=0x7f2f270ab47c <__func__.13609>) at ../../module/zfs/zap_micro.c:573
#10 0x00007f2f27022054 in zap_expand_leaf (zn=zn@entry=0x7f2f185752d0, l=0x28c3a20, tag=tag@entry=0x7f2f270ab47c <__func__.13609>, tx=tx@entry=0x7f2f18801e90, lp=lp@entry=0x7f2ee33cd8a8) at ../../module/zfs/zap.c:644
#11 0x00007f2f27022bf7 in fzap_add_cd (zn=zn@entry=0x7f2f185752d0, integer_size=integer_size@entry=8, num_integers=num_integers@entry=512, val=val@entry=0x28b1cb0, cd=cd@entry=4294967295, tag=tag@entry=0x7f2f270ab47c <__func__.13609>, tx=tx@entry=0x7f2f18801e90) at ../../module/zfs/zap.c:853
#12 0x00007f2f27022d5e in fzap_add (zn=zn@entry=0x7f2f185752d0, integer_size=integer_size@entry=8, num_integers=num_integers@entry=512, val=val@entry=0x28b1cb0, tag=tag@entry=0x7f2f270ab47c <__func__.13609>, tx=tx@entry=0x7f2f18801e90) at ../../module/zfs/zap.c:874
#13 0x00007f2f270296ad in zap_add_impl (zap=0x28c3440, key=key@entry=0x7f2ee33cdc00 "DDT-sha512-zap-duplicate", integer_size=integer_size@entry=8, num_integers=num_integers@entry=512, val=val@entry=0x28b1cb0, tx=tx@entry=0x7f2f18801e90) at ../../module/zfs/zap_micro.c:1151
#14 0x00007f2f2702a8b8 in zap_add (os=os@entry=0x2835040, zapobj=<optimized out>, key=key@entry=0x7f2ee33cdc00 "DDT-sha512-zap-duplicate", integer_size=integer_size@entry=8, num_integers=num_integers@entry=512, val=val@entry=0x28b1cb0, tx=tx@entry=0x7f2f18801e90) at ../../module/zfs/zap_micro.c:1179
#15 0x00007f2f26f2657f in ddt_object_create (type=DDT_TYPE_ZAP, tx=0x7f2f18801e90, class=DDT_CLASS_DUPLICATE, ddt=0x28b0bf0) at ../../module/zfs/ddt.c:79
#16 ddt_sync_entry (txg=344, tx=0x7f2f18801e90, dde=0x7f2f188dfcd0, ddt=0x28b0bf0) at ../../module/zfs/ddt.c:1133
#17 ddt_sync_table (txg=344, tx=0x7f2f18801e90, ddt=0x28b0bf0) at ../../module/zfs/ddt.c:1171
#18 ddt_sync (spa=spa@entry=0x27d73a0, txg=txg@entry=344) at ../../module/zfs/ddt.c:1212
#19 0x00007f2f26f920b9 in spa_sync (spa=spa@entry=0x27d73a0, txg=txg@entry=344) at ../../module/zfs/spa.c:6602
#20 0x00007f2f26fa5918 in txg_sync_thread (dp=0x281a750) at ../../module/zfs/txg.c:545
#21 0x00007f2f26ee8294 in zk_thread_helper (arg=0x28e45e0) at kernel.c:140
#22 0x00007f2f25ce9dc5 in start_thread () from /lib64/libpthread.so.0
#23 0x00007f2f25a16ced in clone () from /lib64/libc.so.6

bzzz77 · 2016-12-12T15:42:27Z

thanks for the reviews. I've extended the coverage of the patch a bit. not sure though how to refresh the PR to reference the updated patch. the panic Brian reported above should be fixed in the new version.

gmelikov · 2016-12-12T18:56:45Z

@bzzz77 you can do a rebase (squash), and push changes to github with -f param (force).

behlendorf · 2016-12-12T19:13:23Z

@bzzz77 please also make the patch is rebased against the very latest master and run make cstyle before pushing it to locally run the cstyle checker.

bzzz77 · 2016-12-12T19:20:08Z

Brian, I rebased against the latest master, did run make cstyle and tested with ztest locally. then made a forced push.

behlendorf · 2016-12-12T22:51:57Z

@bzzz77 due to the very recent 02730c3 commit you'll need to run autogen.sh again to enable the picky cstyle checks. Then you should be able to reproduce these warnings locally. It looks like we're still getting the same ztest failure in the latest round of test runs too.

bzzz77 · 2016-12-13T06:31:41Z

@behlendorf, I did like you saied and pushed again.. probably I'm doing something wrong, but I see the changes committed, including zap_add_impl() taking 'tag' as a parameter - this was missed in the initial patch and caused panic in zloop testing.

behlendorf · 2016-12-13T18:58:50Z

Almost there. There updated PR just includes an extra patch which is already part of master. So just rebase on the latest master and force update the PR.

# Pull the latest version of master.
git checkout master
git pull zfsonlinux master

# Checkout your branch and rebase on the latest master.
git checkout bydnode-access
git rebase -i master # Drop any extraneous patches

# Force push your updated branch to GitHub to trigger a fresh test run.
git push --force bzzz77  bydnode-access

Before pushing you'll want to fix this warning in dmu_read_impl. By default warnings are treated as errors in the debug builds (./configure --enable-debug) so this was fatal.

../../module/zfs/dmu.c: In function 'dmu_read_impl':
../../module/zfs/dmu.c:875:2: error: 'err' may be used uninitialized in this function [-Werror=maybe-uninitialized]
  return (err);
  ^
cc1: all warnings being treated as errors

bzzz77 · 2016-12-14T07:53:55Z

thanks, I'm updating the patch..
btw, dmu_tx_t is a cache of oid->dnode related to the transaction. it's tiny, it's local and it's exclusive. so we can lookup dnode_t there. do you think we can do this internally in zap_add() and similar methods? then we don't need to introduce zap_add_by_dnode(), etc.

ahrens · 2016-12-16T19:28:06Z

@bzzz77

dmu_tx_t is a cache of oid->dnode related to the transaction. it's tiny, it's local and it's exclusive. so we can lookup dnode_t there. do you think we can do this internally in zap_add() and similar methods? then we don't need to introduce zap_add_by_dnode(), etc.

That could work to get us most of the benefit with less code change. Obviously it doesn't help for zap_lookup* which doesn't take a dmu_tx_t. I'm open to either approach. Do you want to code up the tx-cache and compare the performance to the _by_dnode approach?

bzzz77 · 2016-12-16T19:36:30Z

sure, I can develop a proto.

bzzz77 · 2016-12-26T16:58:05Z

in my benchmarks I see that lookup-in-tx approach is ~0.5% slower compared to direct by-dnode. so i guess it's better to leave the patch as is.

behlendorf · 2016-12-27T17:13:03Z

@bzzz77 did you mean to close this? From your comment above it sounds like the direct by-dnode patch is the way to go.

bzzz77 · 2016-12-28T11:57:08Z

direct by-dnode is what this patch is doing - few methods added like zap_add_by_dnode(), etc. so I'd rather like to see it landed when possible :)

behlendorf · 2016-12-28T14:29:26Z

Right, then let's get this reviewed and merged. Can you reopen this PR, rebase the latest version of the patch against master and force update the branch on Github.

ahrens approved these changes Dec 9, 2016

View reviewed changes

bzzz77 force-pushed the bydnode-access branch from 19a183c to 6df37e9 Compare December 12, 2016 15:30

bzzz77 force-pushed the bydnode-access branch from 6df37e9 to 8254657 Compare December 12, 2016 19:19

bzzz77 force-pushed the bydnode-access branch from 8254657 to bef8068 Compare December 13, 2016 06:24

bzzz77 force-pushed the bydnode-access branch from bef8068 to c15b3ce Compare December 13, 2016 19:29

bzzz77 force-pushed the bydnode-access branch from c15b3ce to 3d6f26b Compare December 14, 2016 08:08

bzzz77 closed this Dec 27, 2016

bzzz77 force-pushed the bydnode-access branch from 3d6f26b to 3500a14 Compare December 27, 2016 08:19

ahrens mentioned this pull request Jan 2, 2017

by-dnode methods let the caller save on dnode#->dnode_t lookup and im… #5534

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

few ZAP methods to access by dnode #5464

few ZAP methods to access by dnode #5464

bzzz77 commented Dec 9, 2016

mention-bot commented Dec 9, 2016

ahrens left a comment

ahrens Dec 9, 2016

ahrens Dec 9, 2016

ahrens Dec 9, 2016

ahrens Dec 9, 2016

ahrens Dec 9, 2016

ahrens Dec 9, 2016

ahrens Dec 9, 2016

behlendorf commented Dec 9, 2016

jxiong commented Dec 9, 2016

behlendorf commented Dec 9, 2016

bzzz77 commented Dec 12, 2016 •

edited

gmelikov commented Dec 12, 2016

behlendorf commented Dec 12, 2016

bzzz77 commented Dec 12, 2016

behlendorf commented Dec 12, 2016

bzzz77 commented Dec 13, 2016

behlendorf commented Dec 13, 2016

bzzz77 commented Dec 14, 2016

ahrens commented Dec 16, 2016

bzzz77 commented Dec 16, 2016

bzzz77 commented Dec 26, 2016

behlendorf commented Dec 27, 2016

bzzz77 commented Dec 28, 2016

behlendorf commented Dec 28, 2016

few ZAP methods to access by dnode #5464

few ZAP methods to access by dnode #5464

Conversation

bzzz77 commented Dec 9, 2016

mention-bot commented Dec 9, 2016

ahrens left a comment

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

ahrens Dec 9, 2016

Choose a reason for hiding this comment

behlendorf commented Dec 9, 2016

jxiong commented Dec 9, 2016

behlendorf commented Dec 9, 2016

bzzz77 commented Dec 12, 2016 • edited

gmelikov commented Dec 12, 2016

behlendorf commented Dec 12, 2016

bzzz77 commented Dec 12, 2016

behlendorf commented Dec 12, 2016

bzzz77 commented Dec 13, 2016

behlendorf commented Dec 13, 2016

bzzz77 commented Dec 14, 2016

ahrens commented Dec 16, 2016

bzzz77 commented Dec 16, 2016

bzzz77 commented Dec 26, 2016

behlendorf commented Dec 27, 2016

bzzz77 commented Dec 28, 2016

behlendorf commented Dec 28, 2016

bzzz77 commented Dec 12, 2016 •

edited