[LU-7239] mdd_attr_set() synchronous when it need not be

• Status: Resolved
• Project: Lustre
• Affects Version/s: Lustre 2.7.0, Lustre 2.5.4
• Fix Version/s: Lustre 2.13.0
• Type: Bug
• Priority: Minor
• Reporter: Olaf Faaland
• Assignee: Hongchao Zhang
• Resolution: Fixed
• Labels: llnl, patch, zfs
• Severity: 3

Description

chmod() and friends are sometimes synchronous when they do not need to be.

We encountered this with teams who use unix groups to allow shared file access. When rsync'ing (or extracting from tar, etc.) the creation of the directory tree on the destination was a series of mkdir() followed by fchownat() calls. A parent directory within the tree had S_ISGID set, and so after mkdir(child), the child also had S_ISGID set; but the subsequent fchownat(child, S_ISGID|some_mode) was forced to be synchronous by the current code.

With regards to S_ISVTX | S_ISUID | S_ISGID, the current code causes mdt_attr_set() to be synchronous when any of those bits are set in the new mode, even if they were set already. The code should check differences between old and new modes, as it already does for bits in S_IRWXUGO.

Some handling is also inconsistent with the intent of permission_needs_sync(), making the chmod operation synchronous when permissions are tightened on a directory.

For S_ISUID, permissions are relaxed when the bit is set, in the sense that the user is allowed to create a file/directory owned by another user. So removing the bit is tightening restrictions.

For S_ISGID, nothing is relaxed or tightened - the bit only takes effect if the user could have done the equivalent chown() anyway. So I believe this bit should not be considered at all.

Issue Links

• is related to: LU-3671 why are permission changes synchronous?
• is related to: LU-11303 slow chgrp as user when quotas are en...

Comments

[Olaf Faaland]

I will submit a patch for the changes described in the description.

I also propose that the default value of the sync_permission flag be reversed. As mentioned in LU-3671, when sync_permission is 0, the filesystem meets POSIX requirements. The performance hit for making mdd_attr_set() synchronous is very great in some scenarios, and although it's certainly more likely a user will noticed a failure on a local filesystem than they will on one node of a distributed one, that logic applies to any operation, not just chown or chmod.

[Joseph Gmitter (Inactive)]

Hi Bobijam,

Can you take a look at the patch when it is submitted?

Thanks.
Joe

[Andreas Dilger]

One factor that contributes significantly here is that TXG sync for ZFS is extremely slow (at most once per second). Conversely, this is basically a non-issue for ldiskfs MDTs. Improving the ZFS sync performance (via ZIL LU-4009) or having ZFS/osd-zfs actually start a sync on the TXG in dt_sync() rather than just calling txg_wait_synced().

[Gerrit Updater]

Olaf Faaland-LLNL ([e-mail]) uploaded a new patch: http://review.whamcloud.com/16699
Subject: LU-7239 mdd: make mdd_attr_set() synchronous less often
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 46cfac7c97dec8348ec17d334f5b9aa3041b0625

[Christopher Morrone]

Yes, of course the ZIL can make sync faster. Even better than having the ZIL for synchronous operations is not needing the ZIL in the first place. Alex talked about this sort of thing in his recent LAD'15 talk.

This seems like an area where the choice to go synchronous is worth revisiting since the performance impact is significant.

[Olaf Faaland]

I notice also that https://jira.whamcloud.com/browse/LUDOC-180 is still open and sync_permission does not appear in the operations manual.

[Gerrit Updater]

Oleg Drokin ([e-mail]) merged in patch https://review.whamcloud.com/16699/
Subject: LU-7239 mdd: make mdd_attr_set() synchronous less often
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: f8e9fb6e10f5b10a83c85b26f146380dd91cb384

[Peter Jones]

Landed for 2.13