Project

General

Profile

Bug #19583

mds: change_attr not inc in Server::handle_set_vxattr

Added by Patrick Donnelly about 1 month ago. Updated 29 days ago.

Status:
Resolved
Priority:
Normal
Category:
-
Target version:
-
Start date:
04/11/2017
Due date:
% Done:

0%

Source:
Development
Tags:
Backport:
Regression:
No
Severity:
3 - minor
Reviewed:
Affected Versions:
ceph-qa-suite:
Release:
Component(FS):
MDS
Needs Doc:
No

Description

Noticed this was missing, was this intentional?

History

#1 Updated by Jeff Layton about 1 month ago

I think you're right -- well spotted! In particular, we need to bump it in the case where we update the ctime (ceph.file.layout only so far).

That said, now that I look. Should we be updating the ctime and change_attr in the ceph.dir.layout case as well? And why not for ceph.quota ? It is a metadata change so it seems like we should do it for all of them.

Thoughts?

#2 Updated by Patrick Donnelly about 1 month ago

Should we be updating the ctime and change_attr in the ceph.dir.layout case as well?

Did you mean mtime?

I think updating mtime and change_attr for vxattr changes makes sense.

#3 Updated by Jeff Layton about 1 month ago

No, I mean the ctime. You only want to update the mtime if you're changing the directory's contents. I would consider the layout to be a a metadata change, so I think you just want to update the ctime and the change_attr.

#4 Updated by John Spray about 1 month ago

Intuitively I would say that things we expose as xattrs should probably bump ctime when they're set, if we're being consistent about pretending that they're posix-ish file metadata.

I guess the counter argument would be that if we're dealing with an rsync-like tool which will in practice ignore the ceph.* attrs, then it's annoying if we bump ctime if there isn't really anything they need to update.

Given how rare our changes the user-modifiable ceph.* attrs are though, that rsync case is probably not painful in practice, probably?

#5 Updated by Jeff Layton about 1 month ago

John Spray wrote:

Intuitively I would say that things we expose as xattrs should probably bump ctime when they're set, if we're being consistent about pretending that they're posix-ish file metadata.

Yes. That sort of thing is rather nice when you consider stuff like intrusion detection, which likes to be able to tell when someone has monkeyed with the file.

I guess the counter argument would be that if we're dealing with an rsync-like tool which will in practice ignore the ceph.* attrs, then it's annoying if we bump ctime if there isn't really anything they need to update.

Given how rare our changes the user-modifiable ceph.* attrs are though, that rsync case is probably not painful in practice, probably?

Yeah, seems like a minor concern.

In any case, I'm not sure rsync would actually do anything there once it looked at the file and tried to calculate the delta. In practice you can't set ctime to a particular value using normal POSIX APIs. The act of changing the ctime is itself a ctime change, and so you just end up with it always marching into the future for any given inode.

#6 Updated by Jeff Layton about 1 month ago

  • Assignee changed from Jeff Layton to Patrick Donnelly

Patrick was going to spin up a patch for this, so reassigning to him. Patrick, if I have that wrong, then please just reassign it back to me and I'll take a look.

#7 Updated by Patrick Donnelly about 1 month ago

  • Status changed from New to Need Review

#8 Updated by John Spray 29 days ago

  • Status changed from Need Review to Resolved

Also available in: Atom PDF