super micro improvemens in posix translator. #4338
Conversation
These were spotted trying to better understand the readdir code. They are trivial and frankly, will save only a handful of cycles. Signed-off-by: Jaco Kroon <jaco@uls.co.za>
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
CLANG-FORMAT FAILURE: index 4b523328b..d1cddaf8a 100644
--- a/xlators/storage/posix/src/posix-inode-fd-ops.c
+++ b/xlators/storage/posix/src/posix-inode-fd-ops.c
@@ -6032,10 +6032,7 @@ posix_readdirp(call_frame_t *frame, xlator_t *this, fd_t *fd, size_t size,
if (op_ret >= 0) {
op_ret = 0;
- list_for_each_entry(entry, &entries.list, list)
- {
- op_ret++;
- }
+ list_for_each_entry(entry, &entries.list, list) { op_ret++; }
}
STACK_UNWIND_STRICT(readdirp, frame, op_ret, op_errno, &entries, NULL); |
| @@ -3349,9 +3348,8 @@ posix_links_in_same_directory(char *dirpath, int count, inode_t *leaf_inode, | |||
| } | |||
|
|
|||
| while (count > 0) { | |||
| errno = 0; | |||
There was a problem hiding this comment.
Are you sure errno is 0 before the system call?
There was a problem hiding this comment.
it doesn't matter, we don't require it afterwards. The only time readdir() will change errno is if the return value is also NULL, so we just need to check the return value. After this we execute closedir(), which will set errno if it returns -1.
| @@ -5823,7 +5819,7 @@ posix_fill_readdir(fd_t *fd, struct posix_fd *pfd, off_t off, size_t size, | |||
| count++; | |||
| } | |||
|
|
|||
| if ((!sys_readdir(pfd->dir, scratch) && (errno == 0))) { | |||
| if (!entry && (errno == 0)) { | |||
There was a problem hiding this comment.
This is the one that should be a bit more head scratching. The loop can come out on three conditions:
- The while condition (which we can as far as I can tell replace with while(1) since filled can never be > size (based on 2, or <= should be < ...).
- We can't add the next entry due to filled + this_size > size (ie, overflowing)
- Hitting EOF.
EOF is the ONLY case where the readdir() inside the loop will return NULL (and errno still == 0).
So there is no point in performing another extra readdir() here.
I trust that the semantics of glusterfs here is that we need to set errno=ENOENT to indicate EOF, rather than merely return a count of 0? Anyway, I don't think this particularly matters, this block indentation will execute once per full readdir() cycle through a folder, compared to some other things.
That said, all of these changes are marginal improvements at best, although, the dropped memsets can in the case of DT_UNKNOWN probably be significant. But I've not encountered DT_UNKNOWN recently anyway it still needs to be covered.
There was a problem hiding this comment.
DT_UNKNOWN is the least used path (as well as the change that is correct, but relevant for BSD only, which is why I did not change it).
These were spotted trying to better understand the readdir code. They are trivial and frankly, will save only a handful of cycles.