* [PATCH] filesetup: do not ask O_RDWR for read-only workload [not found] <CGME20230203123513epcas5p4cb1961d7d1376bf39690f82de639d6f4@epcas5p4.samsung.com> @ 2023-02-03 12:34 ` Kanchan Joshi 2023-02-03 12:46 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Kanchan Joshi @ 2023-02-03 12:34 UTC (permalink / raw) To: fio, axboe, vincent.fu; +Cc: Kanchan Joshi 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; if (is_std) f->fd = dup(STDIN_FILENO); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 12:34 ` [PATCH] filesetup: do not ask O_RDWR for read-only workload Kanchan Joshi @ 2023-02-03 12:46 ` Jens Axboe 2023-02-03 14:08 ` Vincent Fu 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2023-02-03 12:46 UTC (permalink / raw) To: Kanchan Joshi; +Cc: fio, vincent.fu 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 12:46 ` Jens Axboe @ 2023-02-03 14:08 ` Vincent Fu 2023-02-03 14:14 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Vincent Fu @ 2023-02-03 14:08 UTC (permalink / raw) To: Jens Axboe, Kanchan Joshi; +Cc: fio, vincent.fu 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? Vincent ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 14:08 ` Vincent Fu @ 2023-02-03 14:14 ` Jens Axboe 2023-02-03 15:26 ` Vincent Fu 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2023-02-03 14:14 UTC (permalink / raw) To: Vincent Fu, Kanchan Joshi; +Cc: fio, vincent.fu 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. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 14:14 ` Jens Axboe @ 2023-02-03 15:26 ` Vincent Fu 2023-02-03 15:32 ` Jens Axboe 2023-02-06 6:52 ` Kanchan Joshi 0 siblings, 2 replies; 10+ messages in thread From: Vincent Fu @ 2023-02-03 15:26 UTC (permalink / raw) To: Jens Axboe, Kanchan Joshi; +Cc: fio, vincent.fu 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 */ }; /* -- ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 15:26 ` Vincent Fu @ 2023-02-03 15:32 ` Jens Axboe 2023-02-06 6:52 ` Kanchan Joshi 1 sibling, 0 replies; 10+ messages in thread From: Jens Axboe @ 2023-02-03 15:32 UTC (permalink / raw) To: Vincent Fu, Kanchan Joshi; +Cc: fio, vincent.fu On 2/3/23 8:26?AM, Vincent Fu wrote: > 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 */ > }; > > /* Looks good to me. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-03 15:26 ` Vincent Fu 2023-02-03 15:32 ` Jens Axboe @ 2023-02-06 6:52 ` Kanchan Joshi 2023-02-06 19:37 ` Jens Axboe 1 sibling, 1 reply; 10+ messages in thread From: Kanchan Joshi @ 2023-02-06 6:52 UTC (permalink / raw) To: Vincent Fu; +Cc: Jens Axboe, fio, vincent.fu [-- Attachment #1: Type: text/plain, Size: 2611 bytes --] On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote: >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://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen Tried, this works, thanks! The patch looks good. Seems this flag can be added to libzbc engine too. That also treats the the char-type open in the same way. [-- Attachment #2: Type: text/plain, Size: 0 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-06 6:52 ` Kanchan Joshi @ 2023-02-06 19:37 ` Jens Axboe 2023-02-07 16:16 ` Vincent Fu 0 siblings, 1 reply; 10+ messages in thread From: Jens Axboe @ 2023-02-06 19:37 UTC (permalink / raw) To: Kanchan Joshi, Vincent Fu; +Cc: fio, vincent.fu On 2/5/23 11:52?PM, Kanchan Joshi wrote: > On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote: >> 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://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen > > Tried, this works, thanks! > The patch looks good. Seems this flag can be added to libzbc engine too. > That also treats the the char-type open in the same way. Thanks for testing - I've added it for libzbc as well. -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-06 19:37 ` Jens Axboe @ 2023-02-07 16:16 ` Vincent Fu 2023-02-07 16:33 ` Jens Axboe 0 siblings, 1 reply; 10+ messages in thread From: Vincent Fu @ 2023-02-07 16:16 UTC (permalink / raw) To: Jens Axboe, Kanchan Joshi; +Cc: fio, vincent.fu On 2/6/23 14:37, Jens Axboe wrote: > On 2/5/23 11:52?PM, Kanchan Joshi wrote: >> On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote: >>> 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://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen >> >> Tried, this works, thanks! >> The patch looks good. Seems this flag can be added to libzbc engine too. >> That also treats the the char-type open in the same way. > > Thanks for testing - I've added it for libzbc as well. > The new flag only affects generic_file_open() but the libzbc ioengine has its own file open function. So the flag has no effect. Moreover libzbc uses the SG_IO ioctl even for character devices. So the code in libzbc_open_dev() (copied from generic_file_open) that opens character devices with O_RDWR is actually unnecessary. I've committed some patches to tidy things up. Vincent ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] filesetup: do not ask O_RDWR for read-only workload 2023-02-07 16:16 ` Vincent Fu @ 2023-02-07 16:33 ` Jens Axboe 0 siblings, 0 replies; 10+ messages in thread From: Jens Axboe @ 2023-02-07 16:33 UTC (permalink / raw) To: Vincent Fu, Kanchan Joshi; +Cc: fio, vincent.fu On 2/7/23 9:16?AM, Vincent Fu wrote: > On 2/6/23 14:37, Jens Axboe wrote: >> On 2/5/23 11:52?PM, Kanchan Joshi wrote: >>> On Fri, Feb 03, 2023 at 10:26:15AM -0500, Vincent Fu wrote: >>>> 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://protect2.fireeye.com/v1/url?k=37173803-566c928b-3716b34c-74fe4860018a-aac6167af74310c3&q=1&e=3e678cf7-7d3f-487a-8a94-a9f0b41ca778&u=https%3A%2F%2Fgithub.com%2Fvincentkfu%2Ffio%2Ftree%2Frwopen >>> >>> Tried, this works, thanks! >>> The patch looks good. Seems this flag can be added to libzbc engine too. >>> That also treats the the char-type open in the same way. >> >> Thanks for testing - I've added it for libzbc as well. >> > > The new flag only affects generic_file_open() but the libzbc ioengine > has its own file open function. So the flag has no effect. That's a good point, not sure how I missed that when I explicitly checked sg for it last week... > Moreover libzbc uses the SG_IO ioctl even for character devices. So > the code in libzbc_open_dev() (copied from generic_file_open) that > opens character devices with O_RDWR is actually unnecessary. > > I've committed some patches to tidy things up. Thanks! -- Jens Axboe ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-02-07 16:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230203123513epcas5p4cb1961d7d1376bf39690f82de639d6f4@epcas5p4.samsung.com>
2023-02-03 12:34 ` [PATCH] filesetup: do not ask O_RDWR for read-only workload Kanchan Joshi
2023-02-03 12:46 ` Jens Axboe
2023-02-03 14:08 ` Vincent Fu
2023-02-03 14:14 ` Jens Axboe
2023-02-03 15:26 ` Vincent Fu
2023-02-03 15:32 ` Jens Axboe
2023-02-06 6:52 ` Kanchan Joshi
2023-02-06 19:37 ` Jens Axboe
2023-02-07 16:16 ` Vincent Fu
2023-02-07 16:33 ` Jens Axboe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.