Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload

[Vincent Fu <vincentfu@gmail.com>]

On 2/3/23 09:14, Jens Axboe wrote:
> On 2/3/23 7:08?AM, Vincent Fu wrote:
>> On 2/3/23 07:46, Jens Axboe wrote:
>>> On Feb 3, 2023, at 5:35 AM, Kanchan Joshi <joshi.k@samsung.com> wrote:
>>>>
>>>> ?Use O_RDONLY flag when read is requested on char-type files.
>>>>
>>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>
>>>> ---
>>>>
>>>> Before this patch: unexpected permission-denial for unprivileged-user.
>>>>
>>>> $ ls -l /dev/ng0n1
>>>> cr--r--r-- 1 root root 242, 0 Feb  3 16:30 /dev/ng0n1
>>>>
>>>> $ ./fio -iodepth=1 -rw=randread -ioengine=io_uring_cmd -bs=4k -numjobs=1
>>>> -size=4k -cmd_type=nvme -filename=/dev/ng0n1 -name=t
>>>> t: (g=0): rw=randread, bs=(R) 4096B-4096B, (W) 4096B-4096B, (T)
>>>> 4096B-4096B, ioengine=io_uring_cmd, iodepth=1
>>>> fio-3.33-71-g7d7a
>>>> Starting 1 process
>>>> fio: pid=131312, err=13/file:filesetup.c:805, func=open(/dev/ng0n1),
>>>> error=Permission denied
>>>>
>>>> filesetup.c | 5 +----
>>>> 1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/filesetup.c b/filesetup.c
>>>> index 1d3cc5ad..d77b8ba4 100644
>>>> --- a/filesetup.c
>>>> +++ b/filesetup.c
>>>> @@ -768,10 +768,7 @@ open_again:
>>>>          else
>>>>              from_hash = file_lookup_open(f, flags);
>>>>      } else if (td_read(td)) {
>>>> -        if (f->filetype == FIO_TYPE_CHAR && !read_only)
>>>> -            flags |= O_RDWR;
>>>> -        else
>>>> -            flags |= O_RDONLY;
>>>> +        flags |= O_RDONLY;
>>>
>>> This will break sg like interfaces, where a read is done by writing the command to the char device.
>>>
>>> ?
>>> Jens Axboe
>>>
>>
>> Kanchan, does it work if you run fio with the --readonly option?
> 
> I think we should just make it work. Conceptually, the patch is obviously
> fine, it just happens to break some oddball cases like sg/bsg. But maybe
> we just add a fio ioengine flag for that, like FIO_RO_NEEDS_RW_OPEN or
> something, where the engine can tell us if it needs a writeable open
> even or a read-only workload.
> 
> Then generic_file_open() can use O_RDONLY for td_read(), except if the
> engine has FIO_RO_NEEDS_RW_OPEN set.
> 

Kanchan, can you try the diff below? It's also available at 
https://github.com/vincentkfu/fio/tree/rwopen

Vincent

diff --git a/engines/sg.c b/engines/sg.c
index 24783374..0bb5be4a 100644
--- a/engines/sg.c
+++ b/engines/sg.c
@@ -1428,7 +1428,7 @@ static struct ioengine_ops ioengine = {
  	.open_file	= fio_sgio_open,
  	.close_file	= fio_sgio_close,
  	.get_file_size	= fio_sgio_get_file_size,
-	.flags		= FIO_SYNCIO | FIO_RAWIO,
+	.flags		= FIO_SYNCIO | FIO_RAWIO | FIO_RO_NEEDS_RW_OPEN,
  	.options	= options,
  	.option_struct_size	= sizeof(struct sg_options)
  };
diff --git a/filesetup.c b/filesetup.c
index 1d3cc5ad..cb7047c5 100644
--- a/filesetup.c
+++ b/filesetup.c
@@ -768,7 +768,7 @@ open_again:
  		else
  			from_hash = file_lookup_open(f, flags);
  	} else if (td_read(td)) {
-		if (f->filetype == FIO_TYPE_CHAR && !read_only)
+		if (td_ioengine_flagged(td, FIO_RO_NEEDS_RW_OPEN) && !read_only)
  			flags |= O_RDWR;
  		else
  			flags |= O_RDONLY;
diff --git a/ioengines.h b/ioengines.h
index d43540d0..2cb9743e 100644
--- a/ioengines.h
+++ b/ioengines.h
@@ -89,6 +89,8 @@ enum fio_ioengine_flags {
  			= 1 << 16,	/* async ioengine with commit function that sets 
issue_time */
  	FIO_SKIPPABLE_IOMEM_ALLOC
  			= 1 << 17,	/* skip iomem_alloc & iomem_free if job sets mem/iomem */
+	FIO_RO_NEEDS_RW_OPEN
+			= 1 << 18,	/* open files in rw mode even if we have a read job */
  };

  /*
--