[LTP] [PATCH v1] fanotify22.c: handle multiple asynchronous error events

[Jan Kara]

On Wed 04-03-26 13:38:07, Wei Gao wrote:
> Since the introduction of the asynchronous fserror reporting framework
> (kernel commit 81d2e13a57c9), fanotify22 has encountered sporadic failures
> due to the non-deterministic nature of event delivery and merging:
> 
> 1) tcase3 failure: A race condition occurs when the test reads the
>    notification fd between two events. Adding a short delay
>    (usleep) ensures all events are dispatched and ready before the
>    read() call.

OK, but please add a comment in the code why this is needed.

> 2) tcase4 failure: The kernel may deliver errors as independent events
>    instead of a single merged event, The test logic is updated to
>    validate the expected error_count by either a single merged event
>    or the accumulation of multiple independent events in the buffer.

Did you investigate why the events didn't get merged in the kernel? If they
are against the same filesystem they should get merged AFAICS.

								Honza

> 
> Reported-by: kernel test robot <oliver.sang@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202602042124.87bd00e3-lkp@intel.com
> Signed-off-by: Wei Gao <wegao@suse.com>
> ---
>  .../kernel/syscalls/fanotify/fanotify22.c     | 32 ++++++++++++++++---
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/fanotify/fanotify22.c b/testcases/kernel/syscalls/fanotify/fanotify22.c
> index 6578474a7..82eed7ba9 100644
> --- a/testcases/kernel/syscalls/fanotify/fanotify22.c
> +++ b/testcases/kernel/syscalls/fanotify/fanotify22.c
> @@ -53,6 +53,8 @@ static struct fanotify_fid_t null_fid;
>  static struct fanotify_fid_t bad_file_fid;
>  static struct fanotify_fid_t bad_link_fid;
>  
> +static int event_count;
> +
>  static void trigger_fs_abort(void)
>  {
>  	SAFE_MOUNT(tst_device->dev, MOUNT_PATH, tst_device->fs_type,
> @@ -88,7 +90,6 @@ static void trigger_bad_link_lookup(void)
>  			ret, BAD_LINK, errno, EUCLEAN);
>  }
>  
> -
>  static void tcase3_trigger(void)
>  {
>  	trigger_bad_link_lookup();
> @@ -176,9 +177,10 @@ static int check_error_event_info_error(struct fanotify_event_info_error *info_e
>  {
>  	int fail = 0;
>  
> -	if (info_error->error_count != ex->error_count) {
> -		tst_res(TFAIL, "%s: Unexpected error_count (%d!=%d)",
> -			ex->name, info_error->error_count, ex->error_count);
> +	if (info_error->error_count != ex->error_count && event_count != ex->error_count) {
> +		tst_res(TFAIL, "%s: Unexpected error_count (%d!=%d && %d!=%d)",
> +			ex->name, info_error->error_count, ex->error_count,
> +			event_count, ex->error_count);
>  		fail++;
>  	}
>  
> @@ -255,8 +257,30 @@ static void do_test(unsigned int i)
>  
>  	tcase->trigger_error();
>  
> +	usleep(100000);
> +

OK, but can you please add a comment why the sleep is here.

>  	read_len = SAFE_READ(0, fd_notify, event_buf, BUF_SIZE);
>  
> +	struct fanotify_event_metadata *metadata;
> +	size_t len = read_len;
> +
> +	event_count = 0;
> +
> +	for (metadata = (struct fanotify_event_metadata *)event_buf;
> +			FAN_EVENT_OK(metadata, len);
> +			metadata = FAN_EVENT_NEXT(metadata, len)) {
> +		event_count++;
> +		struct fanotify_event_info_error *info_error = get_event_info_error(metadata);
> +
> +		if (info_error) {
> +			tst_res(TINFO, "Event [%d]: errno=%d (expected %d), error_count=%d (expected total %d)",
> +					event_count, info_error->error, tcase->error,
> +					info_error->error_count, tcase->error_count);
> +		} else {
> +			tst_res(TINFO, "Event [%d]: No error info record found", event_count);
> +		}
> +	}
> +

This looks too lax to me. I think

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR