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

bzzz77 · 2016-12-28T19:53:25Z

…prove performance (especially multithread metadata ops)

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
Change has been approved by a ZFS on Linux member.

mention-bot · 2016-12-28T19:53:29Z

@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 · 2017-01-02T18:25:03Z

IIRC, this is an updated version of #5464 (which doesn't seem to show the changes anymore)

ahrens

This looks good overall, just needs the few small changes I mentioned.

ahrens · 2017-01-02T18:29:19Z

module/zfs/dmu.c

-dmu_read(objset_t *os, uint64_t object, uint64_t offset, uint64_t size,
-    void *buf, uint32_t flags)
+dmu_read_impl(dnode_t *dn, uint64_t offset, uint64_t size,
+    void *buf, uint32_t flags, void *tag)


I don't think you need the tag argument. This func can continue holding/releasing the dbufs using FTAG, because they are held/released by this function and not any others.

ahrens · 2017-01-02T18:30:36Z

module/zfs/dmu.c

+{
+	int err;
+	err = dmu_read_impl(dn, offset, size, buf, flags, FTAG);
+	return (err);


[nit] not a big deal, but if you want, you could simplify this to:

return (dmu_read_impl(...));

ahrens · 2017-01-02T18:38:46Z

module/zfs/dmu_tx.c

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


I don't understand the if statement here. dmu_tx_hold_dnode_impl() adds its own hold to the dnode. Therefore I think we should always release our hold, if we put one on. We can also use FTAG, since this func is the only one adding/removing its hold on the dnode. So this code should become:

if (object != DMU_NEW_OBJECT) { err = dnode_hold(os, object, FTAG, &dn); ... ... if (dn != NULL) { dnode_rele(dn, FTAG); }

ahrens · 2017-01-02T18:40:47Z

module/zfs/dmu_tx.c

-	}
+	if (!dmu_tx_is_syncing(tx))
+		(void) dmu_tx_hold_dnode_impl(tx, os,
+		    dn, THT_NEWOBJECT, 0, 0);


The Solaris / illumos style guide suggests braces { } around multi-line if bodies even if it's only one statement, for visual readability. But this isn't checked by the cstyle tool. I don't think it's a huge deal but that's why it's there now.

ahrens · 2017-01-02T18:49:18Z

module/zfs/zap_micro.c

+	if (err != 0)
+		return (err);
+	err = zap_add_impl(zap, key, integer_size, num_integers, val, tx, FTAG);
+	if (zap != NULL)	/* may be NULL if fzap_add() failed */


The comment is no longer true. zap is passed by value, so there's no way for it to be changed. You'll need to either pass a pointer to it (&zap), or have zap_add_impl() do the zap_unlockdir() for us. Neither is that elegant, but I think the latter is less gross.

ahrens · 2017-01-02T18:49:26Z

module/zfs/zap_micro.c

+	err = zap_lockdir_by_dnode(dn, tx, RW_WRITER, TRUE, TRUE, FTAG, &zap);
+	if (err != 0)
+		return (err);
+	err = zap_add_impl(zap, key, integer_size, num_integers, val, tx, FTAG);
 	if (zap != NULL)	/* may be NULL if fzap_add() failed */


bzzz77 · 2017-01-09T12:25:28Z

the patch has been updated according to Matt's review. thanks.

ahrens · 2017-01-09T21:21:33Z

module/zfs/zap_micro.c

 	zn = zap_name_alloc(zap, key, MT_EXACT);
 	if (zn == NULL) {
-		zap_unlockdir(zap, FTAG);
-		return (SET_ERROR(ENOTSUP));
+		err = ENOTSUP;


The equivalent of the existing code would be err = SET_ERROR(ENOTSUP);

ahrens · 2017-01-09T21:49:35Z

I've ported this to illumos and opened openzfs/openzfs#276

ahrens · 2017-01-11T19:45:10Z

We should remove the unused os parameter to dmu_tx_hold_dnode_impl()

openzfs/openzfs#276 (comment)

behlendorf

Just a few nit-picky things left to address and this LGTM.

behlendorf · 2017-01-12T23:50:28Z

module/zfs/dmu.c

@@ -823,16 +823,11 @@ dmu_free_range(objset_t *os, uint64_t object, uint64_t offset,
 }

 int


This should be defined as a static int return type.

behlendorf · 2017-01-12T23:51:20Z

module/zfs/dmu.c

+{
+	return (dmu_read_impl(dn, offset, size, buf, flags));
+}
+
 void


This should be defined as a static void return type.

behlendorf · 2017-01-12T23:53:44Z

module/zfs/dmu.c

+	    FALSE, FTAG, &numbufs, &dbp));
+
+	dmu_write_impl(dbp, numbufs, offset, size, buf, tx);
+


Personally I'd remove the lines of extra white space around dmu_write_impl() so this is consistent with the formatting in dmu_read().

behlendorf · 2017-01-12T23:55:09Z

module/zfs/dmu.c

+	VERIFY0(dmu_buf_hold_array_by_dnode(dn, offset, size,
+	    FALSE, FTAG, &numbufs, &dbp, DMU_READ_PREFETCH));
+
+	dmu_write_impl(dbp, numbufs, offset, size, buf, tx);


Same white space comment from dmu_write().

behlendorf · 2017-01-13T00:06:41Z

module/zfs/dmu_tx.c

@@ -637,19 +669,16 @@ dmu_tx_mark_netfree(dmu_tx_t *tx)
 }

 void


static void

behlendorf · 2017-01-13T00:08:52Z

module/zfs/dmu_tx.c

+	(void) dmu_tx_hold_free_impl(txh, off, len);
+}
+
+void


static void

behlendorf · 2017-01-13T00:15:11Z

module/zfs/dmu_tx.c

 EXPORT_SYMBOL(dmu_tx_hold_zap);
+EXPORT_SYMBOL(dmu_tx_hold_zap_by_dnode);
 EXPORT_SYMBOL(dmu_tx_hold_bonus);


There's no matching EXPORT_SYMBOL for dmu_tx_hold_bonus_by_dnode() even though it was added to dmu.h. Let's add one here.

behlendorf · 2017-01-13T00:16:00Z

module/zfs/zap_micro.c

@@ -1289,22 +1316,16 @@ zap_remove(objset_t *os, uint64_t zapobj, const char *name, dmu_tx_t *tx)
 }

 int


static int

…prove performance (especially multithread metadata ops)

bzzz77 · 2017-01-13T06:19:55Z

the patch has been updated according to the reviews. thanks all.

behlendorf

Thanks, this LGTM.

Add *_by_dnode() routines for accessing objects given their dnode_t *, this is more efficient than accessing the object by (objset_t *, uint64_t object). This change converts some but not all of the existing consumers. As performance-sensitive code paths are discovered they should be converted to use these routines. Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Alex Zhuravlev <bzzz@whamcloud.com> Closes openzfs#5534 Issue openzfs#4802

ahrens requested changes Jan 2, 2017

View reviewed changes

bzzz77 force-pushed the bydnode-access branch 3 times, most recently from 1af2daa to 0900622 Compare January 9, 2017 12:23

ahrens approved these changes Jan 9, 2017

View reviewed changes

ahrens mentioned this pull request Jan 9, 2017

7801 add more by-dnode routines openzfs/openzfs#276

Closed

bzzz77 force-pushed the bydnode-access branch from 0900622 to 1c400c9 Compare January 10, 2017 08:56

behlendorf approved these changes Jan 13, 2017

View reviewed changes

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

ccde879

…prove performance (especially multithread metadata ops)

bzzz77 force-pushed the bydnode-access branch from 1c400c9 to ccde879 Compare January 13, 2017 06:19

behlendorf approved these changes Jan 13, 2017

View reviewed changes

behlendorf merged commit 0eef1bd into openzfs:master Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bzzz77 commented Dec 28, 2016

mention-bot commented Dec 28, 2016

ahrens commented Jan 2, 2017 •

edited

Loading

ahrens left a comment

ahrens Jan 2, 2017

ahrens Jan 2, 2017

ahrens Jan 2, 2017

ahrens Jan 2, 2017

ahrens Jan 2, 2017

ahrens Jan 2, 2017

bzzz77 commented Jan 9, 2017

ahrens Jan 9, 2017

ahrens commented Jan 9, 2017

ahrens commented Jan 11, 2017

behlendorf left a comment

behlendorf Jan 12, 2017

behlendorf Jan 12, 2017

behlendorf Jan 12, 2017

behlendorf Jan 12, 2017

behlendorf Jan 13, 2017

behlendorf Jan 13, 2017

behlendorf Jan 13, 2017

behlendorf Jan 13, 2017

bzzz77 commented Jan 13, 2017

behlendorf left a comment

		@@ -823,16 +823,11 @@ dmu_free_range(objset_t *os, uint64_t object, uint64_t offset,
		}

		int

		FALSE, FTAG, &numbufs, &dbp));

		dmu_write_impl(dbp, numbufs, offset, size, buf, tx);

		@@ -637,19 +669,16 @@ dmu_tx_mark_netfree(dmu_tx_t *tx)
		}

		void

		@@ -1289,22 +1316,16 @@ zap_remove(objset_t os, uint64_t zapobj, const char name, dmu_tx_t *tx)
		}

		int

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

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

Conversation

bzzz77 commented Dec 28, 2016

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Dec 28, 2016

ahrens commented Jan 2, 2017 • edited Loading

ahrens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzzz77 commented Jan 9, 2017

Choose a reason for hiding this comment

ahrens commented Jan 9, 2017

ahrens commented Jan 11, 2017

behlendorf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzzz77 commented Jan 13, 2017

behlendorf left a comment

Choose a reason for hiding this comment

ahrens commented Jan 2, 2017 •

edited

Loading