DAFS: Free header on partially-attached vol salv
authorAndrew Deason <adeason@sinenomine.net>
Thu, 4 Oct 2012 19:15:34 +0000 (14:15 -0500)
committerDaria Brashear <shadow@your-file-system.com>
Wed, 21 Jan 2015 15:39:37 +0000 (10:39 -0500)
When we VRequestSalvage_r a volume, normally the header is freed when
the volume goes offline. This happens when we VOfflineForSalvage_r,
either via VCheckSalvage when nUsers drops to 0, or in
VRequestSalvage_r itself if nUsers is already 0. We cannot free the
header under normal circumstances, since someone else may have a ref
on vp, which implies that the vol header object is okay to use.

However, for VOL_SALVAGE_NO_OFFLINE, we skip all of that. For
VOL_SALVAGE_NO_OFFLINE, the volume has only been partially attached,
so it does not go through the full offlining process, so we don't ever
hit the normal VPutVolume_r handlers etc. So, in the current code, we
don't free the header. But our nUsers drops to 0 anyway, and when
nUsers is 0, our header is supposed to be on the LRU (if we have one).
"oops"

Rectify this by freeing the volume header when VOL_SALVAGE_NO_OFFLINE
is set. Add some comments to try to be very clear about what's going
on.

Note that similar behavior was removed in commit
4552dc552687267fce3c7a6a9c7f4a1e9395c8e5 via a similar flag called
VOL_SALVAGE_INVALIDATE_HEADER. I believe now that this is the same
scenario that VOL_SALVAGE_INVALIDATE_HEADER was trying to solve.
However, VOL_SALVAGE_INVALIDATE_HEADER was not always used correctly,
and its purpose was not really adequately explained, which contributed
to the idea that its very existence was buggy.

Previously, when VOL_SALVAGE_INVALIDATE_HEADER existed, it was used
incorrectly in the VRequestSalvage_r calls in GetVolume,
VForceOffline_r, and VAllocBitmapEntry_r. All of these call sites
could have a vp with other references held on it, and so invalidating
the header there can cause segfaults when the header is freed. So
ideally, commit 4552dc552687267fce3c7a6a9c7f4a1e9395c8e5 would have
just removed the flag from those call sites.

This change effectively restores the behavior that
VOL_SALVAGE_INVALIDATE_HEADER provided. But no new flags are gained,
since this behavior is what we want for the VOL_SALVAGE_NO_OFFLINE
flag. This is not a coincidence; for the 'normal' case, we will free
the header whenever we offline the volume. But for the 'do not
offline' case, obviously that will never happen, so we need to do it
separately. So, these two flags are really the same thing.

Change-Id: Ia369850a33c0e781a3ab2f22017b8559410ff8bf
Reviewed-on: http://gerrit.openafs.org/8204
Reviewed-by: Andrew Deason <adeason@sinenomine.net>
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Daria Brashear <shadow@your-file-system.com>

src/vol/volume.c

index 89a9314..9e8db4a 100644 (file)
@@ -5696,6 +5696,37 @@ VRequestSalvage_r(Error * ec, Volume * vp, int reason, int flags)
            *ec = VSALVAGE;
            code = 1;
        }
+       if ((flags & VOL_SALVAGE_NO_OFFLINE)) {
+           /* Here, we free the header for the volume, but make sure to only
+            * do this if VOL_SALVAGE_NO_OFFLINE is specified. The reason for
+            * this requires a bit of explanation.
+            *
+            * Normally, the volume header will be freed when the volume goes
+            * goes offline. However, if VOL_SALVAGE_NO_OFFLINE has been
+            * specified, the volume was in the process of being attached when
+            * we discovered that it needed salvaging. Thus, the volume will
+            * never go offline, since it never went fully online in the first
+            * place. Specifically, we do not call VOfflineForSalvage_r above,
+            * and we never get rid of the volume via VPutVolume_r; the volume
+            * has not been initialized enough for those to work.
+            *
+            * So instead, explicitly free the volume header here. If we do not
+            * do this, we are wasting a header that some other volume could be
+            * using, since the header remains attached to the volume. Also if
+            * we do not free the header here, we end up with a volume where
+            * nUsers == 0, but the volume has a header that is not on the
+            * header LRU. Some code expects that all nUsers == 0 volumes have
+            * their header on the header LRU (or have no header).
+            *
+            * Also note that we must not free the volume header here if
+            * VOL_SALVAGE_NO_OFFLINE is not set. Since, if
+            * VOL_SALVAGE_NO_OFFLINE is not set, someone else may have a
+            * reference to this volume, and they assume they can use the
+            * volume's header. If we free the volume out from under them, they
+            * can easily segfault.
+            */
+           FreeVolumeHeader(vp);
+       }
     }
     return code;
 }