Re: [PATCH] e2fsprogs: error checking in blkid/devname.c

Philip Spencer
2008-02-22T16:23:20Z

>
> This looks good, but I assume that the bug was caused by some race
> condition where if you try to call dm_task_get_info() while some other
> process is creating or removing a snapshot, dm_task_get_info() is
> returning some kind of EAGAIN, or some other "Try again; we're busy"
> error, right?
>
> If that is the case, can you try to find out what error is being
> returned? It may be the right thing to do is to check to see if we
> are getting a "resource is locked; try again in a sec" error message,
> and retry the dm_task_get_info(), instead of just returning a failure.
>
> Thanks!!

[ A copy of my posting to RH Bugzilla]

I (the original poster) know very little about either e2fsprogs or
device-mapper, and had originally just assumed it would be normal for the
info field to be null after a call to DM_DEVICE_DEPS if there were no
dependents, but now after a quick look at the sources I see that the info
field "dmi" inside the task structure is just what is returned by the
ioctl, so it does appear to me now that some sort of error occurred, and
that otherwise it would have returned a non-null dmi with a zero "exists"
flag inside it.

Correct me if I'm wrong, but it seems that:

-- No point in retrying dm_task_get_info(); it is just unpacking the
"dmi" structure returned by the previous dm_task_run call, which is null.
It is in dm_task_run that the error occurred.

-- The code in dm_task_run seems to already take care of retrying EAGAIN
conditions.

-- One obvious other type of race condition would be if the device were
removed in between the task creation and call to dm_task_run. In that
case, Eric's patch seems to do exactly the right thing -- no point in
continuing if the device is gone anyway.

-- But, I don't think that's the race condition we're seeing. A gdb
printout of the task structure shows

{type = 7, dev_name = 0x2aaaaace3e10 "vg1-snapweb-cow", head = 0x0,
tail = 0x0, read_only = 0, event_nr = 0, major = -1, minor = -1, uid = 0,
gid = 6, mode = 432, dmi = {v4 = 0x0, v1 = 0x0}, newname = 0x0,
message = 0x0, geometry = 0x0, sector = 0, no_flush = 0, no_open_count = 0,
skip_lockfs = 0, suppress_identical_reload = 0, uuid = 0x0}

This is associated to the snapshot volume "snapweb" which was being backed
up at the time. Timestamps on the backup logs indicate that my backup
script moved on to the next filesystem 30 seconds AFTER the segfault, so,
unless something really slowed down the system so that deallocation of the
snapweb volume took a full 30 seconds, it does not appear that the
segfault occurred during the unmounting and deallocating of snapweb.

I also don't understand why major/minor are -1 in the above structure; is
that normal?

- Philip

--------------------------------------------+-------------------------------
Philip Spencer pspencer@fields.utoronto.ca | Director of Computing Services
Room 336 (416)-348-9710 ext3036 | The Fields Institute for
222 College St, Toronto ON M5T 3J1 Canada | Research in Mathematical Sciences

On Fri, 22 Feb 2008, Theodore Tso wrote:

> On Thu, Feb 21, 2008 at 04:10:17PM -0600, Eric Sandeen wrote:
>> This is for RH Bugzilla #433857:
>> rpc.mountd segfaults due to uninitialized value in e2fsprogs devname.c
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=433857
>>
>> which did some very helpful analysis & provided a patch.
>>
>> This patch is based on that, but checks all the devicemapper calls,
>> and does some goto error handling / unwrapping, in the same style as
>> the device-mapper lib code itself.
>
> - Ted
>