Bug #319: allows snaps in root directory - CephFS - Ceph

Actions

Copy link

Bug #319

closed

allows snaps in root directory

Added by Sage Weil over 13 years ago. Updated over 7 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Greg Farnum

Category:

Target version:

% Done:

80%

Source:

Tags:

Backport:

Regression:

Severity:

Reviewed:

Affected Versions:

ceph-qa-suite:

Component(FS):

Labels (FS):

Pull request ID:

Crash signature (v1):

Crash signature (v2):

Actions

Copy link

Updated by Sage Weil over 13 years ago

Assignee set to Greg Farnum

Actions

Copy link

Updated by Greg Farnum over 13 years ago

Status changed from New to In Progress

Been working on this today; I haven't run across any big architectural issues so far, just a lot of asserts and bad assumptions. Continuing to work through them.

Actions

Copy link

Updated by Greg Farnum over 13 years ago

I'm reworking SnapRealms a little bit in order to make this work smoothly. Switching them to use the projected_versions stuff we do with CInodes et al.

Actions

Copy link

Updated by Greg Farnum over 13 years ago

Fixed in branch rootsnap, commit:6aec68150aaac3e23ab8cdd1ad91b13b55827aa2. Pending review.
This contains a fairly significant reworking of snaprealms and should get some more documentation, but the code seems to work!

Actions

Copy link

Updated by Sage Weil over 13 years ago

% Done changed from 0 to 80

Yay, so close!

Pushed a couple cleanups that drop the projected xattr/snaprealm pointers.

I'm worried about this hunk

@@ -507,8 +507,10 @@ private:
   inode_t *add_primary_dentry(CDentry *dn, bool dirty, 
                   CInode *in=0, fragtree_t *pdft=0, bufferlist *psnapbl=0,
                   map<string,bufferptr> *px=0) {
-    return add_primary_dentry(add_dir(dn->get_dir(), false),
+    if (dn)
+      return add_primary_dentry(add_dir(dn->get_dir(), false),
                   dn, dirty, in, pdft, psnapbl, px);
+    else return NULL;
   }
   inode_t *add_primary_dentry(dirlump& lump, CDentry *dn, bool dirty, 
                   CInode *in=0, fragtree_t *pdft=0, bufferlist *psnapbl=0,

in commit:0d5829e03baf5102e84cdc62612802008a2f7a48... Instead of it semi-silently doing nothing if the dentry is missing, the caller should call EMetaBlob::add_root() instead, like MDCache::journal_dirty_inode() does.

The real fix, I think, is for mksnap and rmsnap to not do all the metablob updates at all, but to just call journal_dirty_inode() and be done with it. See the _setattr() method:

  ...all the projectiong and predirtying...

  // log + wait
  le->metablob.add_client_req(req->get_reqid(), req->get_oldest_client_tid());
  mdcache->predirty_journal_parents(mdr, &le->metablob, cur, 0, PREDIRTY_PRIMARY, false);
  mdcache->journal_dirty_inode(mdr, &le->metablob, cur);

Actions

Copy link

Updated by Greg Farnum over 13 years ago

Hmmm, I forgot how much of a mess that commit was; I should clean it up a bit (at least to get rid of those commented-out blocks!).

Sage Weil wrote:

I'm worried about this hunk
in commit:0d5829e03baf5102e84cdc62612802008a2f7a48... Instead of it semi-silently doing nothing if the dentry is missing, the caller should call EMetaBlob::add_root() instead, like MDCache::journal_dirty_inode() does.

The real fix, I think, is for mksnap and rmsnap to not do all the metablob updates at all, but to just call journal_dirty_inode() and be done with it. See the _setattr()

Hmmm, as I recall the only situation where this happens is when discussing the root. I think there's something that recurses through parent directories and stops when it reaches root node, but if the parent node is root it doesn't work, and blocking there was somehow difficult. I'll look into a neater solution in the morning!

Actions

Copy link

Updated by Greg Farnum over 13 years ago

All right, best I can tell that silent fallout isn't even needed any more -- removing it still lets me create and delete snapshots in the root dir and a subdir. Must have been rendered useless by some other changes. Removed it.

Actions

Copy link