dht[WIP]: Implement seek fop at dht level #3792

mohit84 · 2022-09-07T04:46:20Z

Before kernel minor version (.24) fuse does not
wind a seek fop but after that fuse winds a seek
fop so implement the fop at dht level.

Fixes: #3373
Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6
Signed-off-by: Mohit Agrawal moagrawa@redhat.com

mohit84 · 2022-09-07T04:46:51Z

/run regression

mykaul · 2022-09-07T05:07:42Z

xlators/cluster/dht/src/dht-inode-read.c

+    int op_errno = EINVAL;
+    off_t offset = 0;
+
+    if (!frame || !frame->local)


Without frame you'll hit

gf_msg("stack", GF_LOG_CRITICAL, 0, LG_MSG_FRAME_ERROR, "!frame"); \

on STACK_UNWIND_STRICT macro.

So I assume this check is not needed.

Even frame->local cannot be NULL. Otherwise we should have returned an error in dht_seek().

amarts · 2022-09-07T06:14:39Z

This is tricky! ie, if a new fop is added to list, shouldn't we be handling it in all the places its required? ie, even in other cluster/ xlators at least?

Do we have this covered in all other places?

mohit84 · 2022-09-07T06:20:24Z

This is tricky! ie, if a new fop is added to list, shouldn't we be handling it in all the places its required? ie, even in other cluster/ xlators at least?

Do we have this covered in all other places?

The other xlators (afr/client/server/posix) already have seek fop.

xhernandez

Please, also add a test case to check that seeks go to the right subvolume and without this patch fail.

xhernandez · 2022-09-07T05:41:04Z

xlators/cluster/dht/src/dht-inode-read.c

+    return 0;
+
+err:
+    op_errno = (op_errno == -1) ? errno : op_errno;


I would prefer to not depend anymore on errno if it's not strictly necessary because it can be easily misused. In this particular case, the macro VALIDATE_OR_GOTO() sets errno before logging a message, which could change errno value or clear it making it completely useless.

Why not just initialize op_errno = EINVAL at line 1743 ? it seems easier and cleaner to me.

This line is not needed anymore.

xhernandez · 2022-09-07T06:06:45Z

xlators/cluster/dht/src/dht-inode-read.c

+    int op_errno = EINVAL;
+    off_t offset = 0;
+
+    if (!frame || !frame->local)


Even frame->local cannot be NULL. Otherwise we should have returned an error in dht_seek().

xhernandez · 2022-09-07T06:44:23Z

xlators/cluster/dht/src/dht-inode-read.c

+        DHT_STACK_UNWIND(seek, frame, -1, local->op_errno, offset,
+                         local->rebalance.xdata);


This is an error, so offset is irrelevant. We normally pass 0 in this case. Also, local->rebalance.xdata doesn't seem to be initialized anywhere. In any case it should be NULL, not the xdata from the request.

xhernandez · 2022-09-07T06:48:45Z

xlators/cluster/dht/src/dht-inode-read.c

+        goto out;
+    }
+
+    if (!op_ret || (local->call_cnt != 1))


There are some error codes that seek can return (for example ENXIO) that I don't think could be related to rebalance. Shouldn't we filter them to avoid the overhead of a synctask and additional fops when it's not really necessary ?

Even better, if we know which error a rebalance can cause, then we could call dht_rebalance_complete_check() just in that case.

mohit84 · 2022-09-07T08:30:42Z

Please, also add a test case to check that seeks go to the right subvolume and without this patch fail.

To create a test case I have to setup an environment where the kernel minor version is >= 24. The test case will not be
useful for the current test server because the minor version is less than 24.

mohit84 · 2022-09-09T06:48:58Z

/run regression

xhernandez · 2022-09-09T08:00:38Z

Please, also add a test case to check that seeks go to the right subvolume and without this patch fail.

To create a test case I have to setup an environment where the kernel minor version is >= 24. The test case will not be useful for the current test server because the minor version is less than 24.

I would write the test anyway, and make sure it works with the proper kernel version. Eventually the test servers will be updated and this test will be useful. Meanwhile it will just pass, which is fine.

mohit84 · 2022-09-09T08:05:16Z

Please, also add a test case to check that seeks go to the right subvolume and without this patch fail.

To create a test case I have to setup an environment where the kernel minor version is >= 24. The test case will not be useful for the current test server because the minor version is less than 24.

I would write the test anyway, and make sure it works with the proper kernel version. Eventually the test servers will be updated and this test will be useful. Meanwhile it will just pass, which is fine.

I have already attached a test case and the test case is tested on fedora 37.

xhernandez · 2022-09-09T08:03:07Z

tests/basic/distribute/dht_seek_test.t

+
+for i in {1..20}; do
+    TEST dd if=/dev/urandom of=${M0}/file.${i} bs=1k count=1
+    TEST dd if=/dev/urandom of=${M0}/file.${i} bs=1k count=1 seek=128


Unless you add conv=notrunc the file will be overwritten, so the previous dd will be lost.

xhernandez · 2022-09-09T08:04:36Z

tests/basic/distribute/dht_seek_test.t

+. $(dirname $0)/../../volume.rc
+. $(dirname $0)/../../dht.rc
+
+SCRIPT_TIMEOUT=300


Does this test really require more than 200 seconds (the default value) to complete ?

xhernandez · 2022-09-09T08:07:39Z

xlators/cluster/dht/src/dht-inode-read.c

+    return 0;
+
+err:
+    op_errno = (op_errno == -1) ? errno : op_errno;


This line is not needed anymore.

xhernandez · 2022-09-09T08:08:37Z

xlators/cluster/dht/src/dht-inode-read.c

+    if (!frame || !this || !fd)
+        goto err;


Do we really need to check that ?

xhernandez · 2022-09-09T08:12:43Z

xlators/cluster/dht/src/dht-inode-read.c

+        goto out;
+    }
+
+    if ((op_errno == ENXIO) || (op_errno == EOVERFLOW))


This can only be tested if op_ret is -1.

Before kernel minor version (.24) fuse does not wind a seek fop but after that fuse winds a seek fop so implement the fop at dht level. Fixes: gluster#3373 Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

Fixes: gluster#3373 Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 · 2022-09-09T08:34:50Z

/run regression

mohit84 · 2022-09-09T12:29:37Z

/run regression

mohit84 · 2022-09-09T15:46:49Z

/run regression

mohit84 · 2022-09-13T09:40:31Z

/run smoke

mohit84 · 2022-09-13T09:50:27Z

/recheck smoke

Before kernel minor version (.24) fuse does not wind a seek fop but after that fuse winds a seek fop so implement the fop at dht level. > Fixes: gluster#3373 > Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 > Signed-off-by: Mohit Agrawal moagrawa@redhat.com > (Reviewed on upstream link gluster#3792) Fixes: gluster#3373 Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 Signed-off-by: Mohit Agrawal moagrawa@redhat.com

Before kernel minor version (.24) fuse does not wind a seek fop but after that fuse winds a seek fop so implement the fop at dht level. > Fixes: #3373 > Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 > Signed-off-by: Mohit Agrawal moagrawa@redhat.com > (Reviewed on upstream link #3792) Fixes: #3373 Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 Signed-off-by: Mohit Agrawal moagrawa@redhat.com Signed-off-by: Mohit Agrawal moagrawa@redhat.com

mykaul reviewed Sep 7, 2022

View reviewed changes

xhernandez reviewed Sep 7, 2022

View reviewed changes

mohit84 force-pushed the issue3373_devel branch from 1eeb5a7 to 11bd385 Compare September 9, 2022 06:47

xhernandez reviewed Sep 9, 2022

View reviewed changes

mohit84 added 2 commits September 9, 2022 14:03

dht: Resolve reviewer comments

d7d85f3

Fixes: gluster#3373 Change-Id: Ie9ef2f941099157996ab353fc4dc208a28fa8fc6 Signed-off-by: Mohit Agrawal <moagrawa@redhat.com>

mohit84 force-pushed the issue3373_devel branch from 11bd385 to d7d85f3 Compare September 9, 2022 08:34

xhernandez approved these changes Sep 13, 2022

View reviewed changes

mohit84 merged commit ab255a8 into gluster:devel Sep 13, 2022

mohit84 mentioned this pull request Sep 13, 2022

dht: Implement seek fop at dht level #3810

Merged

mohit84 mentioned this pull request Sep 13, 2022

dht: Implement seek fop at dht level #3811

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dht[WIP]: Implement seek fop at dht level #3792

dht[WIP]: Implement seek fop at dht level #3792

mohit84 commented Sep 7, 2022

mohit84 commented Sep 7, 2022

mykaul Sep 7, 2022

xhernandez Sep 7, 2022

amarts commented Sep 7, 2022

mohit84 commented Sep 7, 2022

xhernandez left a comment

xhernandez Sep 7, 2022

mohit84 Sep 9, 2022

xhernandez Sep 9, 2022

xhernandez Sep 7, 2022

xhernandez Sep 7, 2022

mohit84 Sep 9, 2022

xhernandez Sep 7, 2022

mohit84 Sep 9, 2022

mohit84 commented Sep 7, 2022

mohit84 commented Sep 9, 2022

xhernandez commented Sep 9, 2022

mohit84 commented Sep 9, 2022

xhernandez Sep 9, 2022

xhernandez Sep 9, 2022

mohit84 Sep 9, 2022

xhernandez Sep 9, 2022

xhernandez Sep 9, 2022

mohit84 Sep 9, 2022

xhernandez Sep 9, 2022

mohit84 Sep 9, 2022

mohit84 commented Sep 9, 2022

mohit84 commented Sep 9, 2022

mohit84 commented Sep 9, 2022

mohit84 commented Sep 13, 2022

mohit84 commented Sep 13, 2022

		DHT_STACK_UNWIND(seek, frame, -1, local->op_errno, offset,
		local->rebalance.xdata);

dht[WIP]: Implement seek fop at dht level #3792

dht[WIP]: Implement seek fop at dht level #3792

Conversation

mohit84 commented Sep 7, 2022

mohit84 commented Sep 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amarts commented Sep 7, 2022

mohit84 commented Sep 7, 2022

xhernandez 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

mohit84 commented Sep 7, 2022

mohit84 commented Sep 9, 2022

xhernandez commented Sep 9, 2022

mohit84 commented Sep 9, 2022

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

mohit84 commented Sep 9, 2022

mohit84 commented Sep 9, 2022

mohit84 commented Sep 9, 2022

mohit84 commented Sep 13, 2022

mohit84 commented Sep 13, 2022