From 82a30ed17c9cf56e319d5e13ca2e18d82f8b239c Mon Sep 17 00:00:00 2001 From: Andrew Deason Date: Thu, 4 Oct 2012 14:15:34 -0500 Subject: [PATCH] DAFS: Free header on partially-attached vol salv 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 Tested-by: BuildBot Reviewed-by: Daria Brashear --- src/vol/volume.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/vol/volume.c b/src/vol/volume.c index 89a9314c9..9e8db4a17 100644 --- a/src/vol/volume.c +++ b/src/vol/volume.c @@ -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; } -- 2.39.5