Remove dummy znode from zvol_state #4510

tuxoko · 2016-04-12T00:17:04Z

struct zvol_state contains a dummy znode, which is around 1KB on x64, only for
zfs_range_lock. But in reality, other than z_range_lock and z_range_avl,
zfs_range_lock only need znode on regular file, which means we add 1KB on a
structure and gain nothing.

In this patch, we remove the dummy znode for zvol_state. In order to do that,
we also need to refactor zfs_range_lock a bit. We move z_range_lock and
z_range_avl pair out of znode_t to form zfs_rlock_t. This new struct replaces
znode_t as the main handle inside the range lock functions. Since regular
files still need znode for RL_WRITER and RL_APPEND, we make znode an optional
argument in zfs_range_lock_impl.

To reduce possible merge conflict, we retain the prototype of zfs_range_lock.
And zvol now should call zvol_range_lock instead.

Signed-off-by: Chunwei Chen david.chen@osnexus.com

behlendorf · 2016-04-12T22:55:51Z

This work nicely complements a patch proposed by @bprotopopov to verify the kernel module range lock implementation in ztest. I've never liked how the znode_t was coupled to the range lock to I'm all for this improvement. But we should coordinate these two changes so we can 1) eliminate the znode_t from the a zvol_state_t, and 2) update ztest to use a single version of the range locks.

The kernel built-in builder also detecting a small build issue where you need to update an existing trace point.

http://build.zfsonlinux.org/builders/Kernel.org%20Built-in%20x86_64%20%28BUILD%29/builds/4012/steps/shell_1/logs/make

tuxoko · 2016-04-13T00:44:06Z

I fixed the trace part. I'm not sure what's the status of #4024. Is it going to be merged? Or is it waiting for illumos?

behlendorf · 2016-04-13T01:15:15Z

The ztest changes are independent of the bug being fixed. Ideally all of these changed should be submitted back upstream.

bprotopopov · 2016-04-13T01:28:45Z

Yes, this could work out quite well; the user-space functions could also be #defined like zvol_*() functions, and the zp_is_zvol member would no linger be needed in the user-space znode.

I can help with the port if needed.

tuxoko · 2016-04-13T01:33:29Z

@bprotopopov Can you rebase the ztest patch on this one, then I'll pull it in this pr.

bprotopopov · 2016-04-13T01:36:25Z

Yes, I can look into it.

behlendorf · 2016-04-22T20:24:47Z

@tuxoko I found some time to get back to this and do a more careful review. I like where this is going, but if we're going to change this code we need to go farther and make it totally generic by eliminating the znode entirely from the range locking code. Let me suggest a way to do this which builds on the work you've already done.

If we assume that range locks will always be embedded in another struct as they are they today. Then we can extend the zfs_rlock_t structure you've introduced to include uint64_t pointers to the z_size, z_blksz, and z_max_blksz fields in the containing structure. This way the zfs_range_lock_writer() function will have safe access to this information for whatever containing structure is using the lock without the znode_t baggage. The zfs_rlock_init() function could be easily extended to take this values.

tuxoko · 2016-04-26T00:46:19Z

@behlendorf
I'm not sure putting pointers in zfs_rlock_t will be any better than having znode in zfs_range_lock_writer. Those pointers and the logic in the function is still very znode specific anyway.

behlendorf · 2016-04-26T03:08:37Z

How so? The logic here is just for appending to a range lock or changing the objects block size. Neither concept is specific to a znode.

bprotopopov · 2016-04-28T16:31:30Z

I have rebased against @tuxoko changes on my branch at https://github.com/bprotopopov/zfs/commits/issue-4023

tuxoko · 2016-05-04T22:10:25Z

Rebase to master, add pointers to z_size and stuff, add ztest patch.

behlendorf · 2016-05-05T17:33:10Z

include/sys/zfs_rlock.h

@@ -60,8 +70,12 @@ typedef struct rl {
 * or exclusive (RL_WRITER or RL_APPEND).  RL_APPEND is a special type that
 * is converted to RL_WRITER that specified to lock from the start of the
 * end of file.  Returns the range lock structure.
+ *
+ * Filesystem should call zfs_range_lock.
+ * Zvol should call zvol_range_lock.


This comment should be removed, there is no zvol_range_lock() and zfs_range_lock() should be usable by all dmu consumers.

bprotopopov · 2016-05-06T15:45:46Z

@tuxoko, I say LGTM.

behlendorf · 2016-05-13T19:50:38Z

@tuxoko @bprotopopov LGTM to me this is a nice bit of cleanup.

I'd suggest exporting the symbols so external consumers can use these range locks. This is functionality that filesystems like Lustre which are layered on the DMU like the ZPL could take advantage of right away. @adilger what are your thoughts on this?

zfs_range_lock()
zfs_range_unlock()
zfs_range_reduce()
zfs_range_compare()

adilger · 2016-05-13T21:24:42Z

@bzzz77 was already looking at adding range locking for the ZFS OSD (http://review.whamcloud.com/15542) so if this was provided directly by ZFS it would avoid duplicating this functionality in Lustre and would definitely be welcome.

adilger · 2016-05-13T21:26:13Z

Also, if the range locks are exported up to the ZPL then it is possible to implement multi-threaded read/write operations from the VFS instead of i_mutex serializing all access to the file.

behlendorf · 2016-05-13T21:40:05Z

In that case, @bzzz77 could you looked over the public interface in zfs_rlock.h and verify it's going to meet your needs. It should.

@tuxoko could you refresh this patch to export the needed symbols. Then this work is ready to merge.

@adilger right, we've always depended on these range lock in the ZPL instead of the i_mutex. It's just that previously they were wired in to the znode a little to tightly which prevented them from being used elsewhere.

struct zvol_state contains a dummy znode, which is around 1KB on x64, only for zfs_range_lock. But in reality, other than z_range_lock and z_range_avl, zfs_range_lock only need znode on regular file, which means we add 1KB on a structure and gain nothing. In this patch, we remove the dummy znode for zvol_state. In order to do that, we also need to refactor zfs_range_lock a bit. We move z_range_lock and z_range_avl pair out of znode_t to form zfs_rlock_t. This new struct replaces znode_t as the main handle inside the range lock functions. We also add pointers to z_size, z_blksz, and z_max_blksz so range lock code doesn't depend on znode_t. Signed-off-by: Chunwei Chen <david.chen@osnexus.com>

Signed-off-by: Chunwei Chen <david.chen@osnexus.com>

tuxoko · 2016-05-13T22:18:56Z

Done

behlendorf · 2016-05-17T17:44:35Z

@tuxoko thanks! Merged as:

e3a07cd Use zfs range locks in ztest
d88895a Remove dummy znode from zvol_state

@bzzz77 let us know if you encounter any issues using the new interface with Lustre. You should be able to use the same approach taken by the ZPL and embed the range lock in your osd object.

struct zvol_state contains a dummy znode, which is around 1KB on x64, only for zfs_range_lock. But in reality, other than z_range_lock and z_range_avl, zfs_range_lock only need znode on regular file, which means we add 1KB on a structure and gain nothing. In this patch, we remove the dummy znode for zvol_state. In order to do that, we also need to refactor zfs_range_lock a bit. We move z_range_lock and z_range_avl pair out of znode_t to form zfs_rlock_t. This new struct replaces znode_t as the main handle inside the range lock functions. We also add pointers to z_size, z_blksz, and z_max_blksz so range lock code doesn't depend on znode_t. This allows non-ZPL consumers like Lustre to use the range locks with their equivalent znode_t structure. Signed-off-by: Chunwei Chen <david.chen@osnexus.com> Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#4510

nedbass · 2016-09-02T19:00:26Z

For future reference, this was added to the 0.6.5.8 milestone so Lustre could take advantage of the ZFS range lock implementation.

struct zvol_state contains a dummy znode, which is around 1KB on x64, only for zfs_range_lock. But in reality, other than z_range_lock and z_range_avl, zfs_range_lock only need znode on regular file, which means we add 1KB on a structure and gain nothing. In this patch, we remove the dummy znode for zvol_state. In order to do that, we also need to refactor zfs_range_lock a bit. We move z_range_lock and z_range_avl pair out of znode_t to form zfs_rlock_t. This new struct replaces znode_t as the main handle inside the range lock functions. We also add pointers to z_size, z_blksz, and z_max_blksz so range lock code doesn't depend on znode_t. This allows non-ZPL consumers like Lustre to use the range locks with their equivalent znode_t structure. Signed-off-by: Chunwei Chen <david.chen@osnexus.com> Signed-off-by: Boris Protopopov <boris.protopopov@actifio.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#4510

tuxoko force-pushed the zrl branch from 2a940ed to c8009a1 Compare April 12, 2016 23:41

tuxoko force-pushed the zrl branch from c8009a1 to 260a3dc Compare May 4, 2016 22:08

behlendorf reviewed May 5, 2016
View reviewed changes

tuxoko force-pushed the zrl branch from 260a3dc to aed2368 Compare May 5, 2016 18:35

Chunwei Chen and others added 3 commits May 13, 2016 15:17

Use zfs range locks in ztest

573bf88

Export zfs_rlock symbols for users like lustre

7a46269

Signed-off-by: Chunwei Chen <david.chen@osnexus.com>

tuxoko force-pushed the zrl branch from c8338c4 to 7a46269 Compare May 13, 2016 22:18

behlendorf closed this in d88895a May 17, 2016

behlendorf added this to the 0.6.5.8 milestone May 17, 2016

tuxoko deleted the zrl branch March 3, 2017 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dummy znode from zvol_state #4510

Remove dummy znode from zvol_state #4510

tuxoko commented Apr 12, 2016

behlendorf commented Apr 12, 2016

tuxoko commented Apr 13, 2016

behlendorf commented Apr 13, 2016

bprotopopov commented Apr 13, 2016

tuxoko commented Apr 13, 2016

bprotopopov commented Apr 13, 2016

behlendorf commented Apr 22, 2016

tuxoko commented Apr 26, 2016

behlendorf commented Apr 26, 2016 •

edited

bprotopopov commented Apr 28, 2016

tuxoko commented May 4, 2016

behlendorf May 5, 2016

bprotopopov commented May 6, 2016

behlendorf commented May 13, 2016 •

edited

adilger commented May 13, 2016

adilger commented May 13, 2016

behlendorf commented May 13, 2016 •

edited

tuxoko commented May 13, 2016

behlendorf commented May 17, 2016

nedbass commented Sep 2, 2016

Remove dummy znode from zvol_state #4510

Remove dummy znode from zvol_state #4510

Conversation

tuxoko commented Apr 12, 2016

behlendorf commented Apr 12, 2016

tuxoko commented Apr 13, 2016

behlendorf commented Apr 13, 2016

bprotopopov commented Apr 13, 2016

tuxoko commented Apr 13, 2016

bprotopopov commented Apr 13, 2016

behlendorf commented Apr 22, 2016

tuxoko commented Apr 26, 2016

behlendorf commented Apr 26, 2016 • edited

bprotopopov commented Apr 28, 2016

tuxoko commented May 4, 2016

behlendorf May 5, 2016

Choose a reason for hiding this comment

bprotopopov commented May 6, 2016

behlendorf commented May 13, 2016 • edited

adilger commented May 13, 2016

adilger commented May 13, 2016

behlendorf commented May 13, 2016 • edited

tuxoko commented May 13, 2016

behlendorf commented May 17, 2016

nedbass commented Sep 2, 2016

behlendorf commented Apr 26, 2016 •

edited

behlendorf commented May 13, 2016 •

edited

behlendorf commented May 13, 2016 •

edited