Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

super micro improvemens in posix translator. #4338

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

super micro improvemens in posix translator. #4338

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions xlators/storage/posix/src/posix-inode-fd-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3244,9 +3244,8 @@ posix_xattr_get_real_filename(call_frame_t *frame, xlator_t *this, loc_t *loc,
fname = key + SLEN(GF_XATTR_GET_REAL_FILENAME_KEY);

for (;;) {
errno = 0;
entry = sys_readdir(fd, scratch);
if (!entry || errno != 0)
if (!entry)
break;

if (strcasecmp(entry->d_name, fname) == 0) {
Expand Down Expand Up @@ -3349,9 +3348,8 @@ posix_links_in_same_directory(char *dirpath, int count, inode_t *leaf_inode,
}

while (count > 0) {
errno = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure errno is 0 before the system call?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

entry = sys_readdir(dirp, scratch);
if (!entry || errno != 0)
if (!entry)
break;

if (entry->d_ino != stbuf->st_ino)
Expand Down Expand Up @@ -5735,9 +5733,9 @@ posix_fill_readdir(fd_t *fd, struct posix_fd *pfd, off_t off, size_t size,
* when the cluster/dht xlator decides to distribute
* exended attribute backing file across storage servers.
*/
if (__is_root_gfid(fd->inode->gfid) == 0 &&
(!strcmp(entry->d_name, ".attribute")))
if (is_root_gfid && (!strcmp(entry->d_name, ".attribute"))) {
continue;
}
#endif /* __NetBSD__ */

if (is_root_gfid && (!strcmp(GF_HIDDEN_PATH, entry->d_name))) {
Expand All @@ -5748,7 +5746,6 @@ posix_fill_readdir(fd_t *fd, struct posix_fd *pfd, off_t off, size_t size,
if (DT_ISDIR(entry->d_type)) {
continue;
} else if (DT_UNKNOWN == entry->d_type) {
memset(&stbuf, 0, sizeof(stbuf));
stat_ret = sys_fstatat(pfd->fd, entry->d_name, &stbuf,
AT_SYMLINK_NOFOLLOW);
if ((stat_ret == 0) && S_ISDIR(stbuf.st_mode))
Expand Down Expand Up @@ -5781,7 +5778,6 @@ posix_fill_readdir(fd_t *fd, struct posix_fd *pfd, off_t off, size_t size,

if (DT_UNKNOWN == entry->d_type) {
if (stat_ret == 1) {
memset(&stbuf, 0, sizeof(stbuf));
stat_ret = sys_fstatat(pfd->fd, entry->d_name, &stbuf,
AT_SYMLINK_NOFOLLOW);
}
Expand Down Expand Up @@ -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)) {
Copy link
Contributor Author

@jkroonza jkroonza Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one that should be a bit more head scratching. The loop can come out on three conditions:

  1. 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 < ...).
  2. We can't add the next entry due to filled + this_size > size (ie, overflowing)
  3. 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

/* Indicate EOF */
errno = ENOENT;
/* Remember EOF offset for later detection */
Expand Down