Re: [PATCH] fix hang when BLKTRACESETUP fails and "-o -" is used

On 6/6/24 6:32 AM, Jeff Moyer wrote:
> Hi, Eric,
> 
> This looks like a repost?  I mentioned that we should also set done = 1
> for a failed setup_tracer_devpaths() call, and I was hoping you'd
> include that in a new version (or at least comment on whether you
> thought it was a good idea).  Sorry if that wasn't clear!

The first one was a question, this one was a patch :)

I'm sorry, I have not had time to look into or test for failed
setup_tracer_devpaths() calls, and hoped to just fix the bug
I had encountered.
 
> The change here looks good to me.

Thanks,
-Eric

> -Jeff
> 
> Eric Sandeen <[email protected]> writes:
> 
>> # blktrace -o - /dev/sda /dev/sdb /dev/sdc
>>
>> has to be SIGKILLed if BLKTRACESETUP fails for any or all of the devices
>> listed. (I simulated this by just catching one of the devices in
>> setup_buts(), skipping the ioctl, and doing ret++).
>>
>> This seems to be because with "-o -" on the command line, use_tracer_devpaths()
>> sees piped_output set, so we call process_trace_bufs which ends up waiting on
>> (!done) and "done" is never set. i.e.
>>
>> atexit(exit_tracing)
>> 	wait_tracers
>> 		if (use_tracer_devpaths()) // true because "-o -"
>> 			process_trace_bufs
>> 				while (wait_empty_entries())
>> 					wait_empty_entries
>> 						while (!done ... )
>> 							<loop forever>
>>
>> I think this can be avoided by just setting "done = 1" before returning
>> when setup_buts() fails in run_tracers().
>>
>> Signed-off-by: Eric Sandeen <[email protected]>
>> ---
>>
>> diff --git a/blktrace.c b/blktrace.c
>> index 3444fbb..038b2cb 100644
>> --- a/blktrace.c
>> +++ b/blktrace.c
>> @@ -2684,8 +2684,10 @@ static int run_tracers(void)
>>  	if (net_mode == Net_client)
>>  		printf("blktrace: connecting to %s\n", hostname);
>>  
>> -	if (setup_buts())
>> +	if (setup_buts()) {
>> +		done = 1;
>>  		return 1;
>> +	}
>>  
>>  	if (use_tracer_devpaths()) {
>>  		if (setup_tracer_devpaths())
> 
>