[PATCH 0/2][SRU][Disco] shiftfs: bugfix and O_DIRECT support

classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2][SRU][Disco] shiftfs: bugfix and O_DIRECT support

Christian Brauner
Hey everyone,

Here's a tiny two-patch series that enables O_DIRECT support for shiftfs
since this is needed for dqlite which is an essential component of LXD
and fixes a bug whereby an unsigned long was passed to copy_from_user()
instead of an void __user *p.

Corresponding SRU justifications are up on Launchpad. The links to them
can be found in the individual patches.

Thanks!
Christian

Christian Brauner (2):
  UBUNTU: SAUCE: shiftfs: add O_DIRECT support
  UBUNTU: SAUCE: shiftfs: pass correct point down

 fs/shiftfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.22.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Christian Brauner
BugLink: https://bugs.launchpad.net/bugs/1837223

This enabled O_DIRECT support for shiftfs if the underlay supports it.

Currently shiftfs does not handle O_DIRECT if the underlay supports it.
This is blocking dqlite - an essential part of LXD - from profiting from
the performance benefits of O_DIRECT on suitable filesystems when used
with async io such as aio or io_uring.
Overlayfs cannot support this directly since the upper filesystem in
overlay can be any filesystem. So if the upper filesystem does not
support O_DIRECT but the lower filesystem does you're out of luck.
Shiftfs does not suffer from the same problem since there is not concept
of an upper filesystem in the same way that overlayfs has it.
Essentially, shiftfs is a transparent shim relaying everything to the
underlay while overlayfs' upper layer is not (completely).

Cc: Seth Forshee <[hidden email]>
Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/shiftfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index 49f6714e9f95..addaa6e21e57 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -1126,6 +1126,9 @@ static int shiftfs_open(struct inode *inode, struct file *file)
  }
 
  file->private_data = file_info;
+ /* For O_DIRECT dentry_open() checks f_mapping->a_ops->direct_IO. */
+ file->f_mapping = realfile->f_mapping;
+
  file_info->realfile = realfile;
  return 0;
 }
--
2.22.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: pass correct point down

Christian Brauner
In reply to this post by Christian Brauner
BugLink: https://bugs.launchpad.net/bugs/1837231

This used to pass an unsigned long to copy_from_user() instead of a
void __user * pointer. This will produce warning with a sufficiently
advanced compiler.

Cc: Seth Forshee <[hidden email]>
Signed-off-by: Christian Brauner <[hidden email]>
---
 fs/shiftfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/shiftfs.c b/fs/shiftfs.c
index addaa6e21e57..9006201c243d 100644
--- a/fs/shiftfs.c
+++ b/fs/shiftfs.c
@@ -1529,7 +1529,7 @@ static bool in_ioctl_whitelist(int flag, unsigned long arg)
  case BTRFS_IOC_SUBVOL_GETFLAGS:
  return true;
  case BTRFS_IOC_SUBVOL_SETFLAGS:
- if (copy_from_user(&flags, arg, sizeof(flags)))
+ if (copy_from_user(&flags, argp, sizeof(flags)))
  return false;
 
  if (flags & ~BTRFS_SUBVOL_RDONLY)
--
2.22.0


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Seth Forshee
In reply to this post by Christian Brauner
On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:

> BugLink: https://bugs.launchpad.net/bugs/1837223
>
> This enabled O_DIRECT support for shiftfs if the underlay supports it.
>
> Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> This is blocking dqlite - an essential part of LXD - from profiting from
> the performance benefits of O_DIRECT on suitable filesystems when used
> with async io such as aio or io_uring.
> Overlayfs cannot support this directly since the upper filesystem in
> overlay can be any filesystem. So if the upper filesystem does not
> support O_DIRECT but the lower filesystem does you're out of luck.
> Shiftfs does not suffer from the same problem since there is not concept
> of an upper filesystem in the same way that overlayfs has it.
> Essentially, shiftfs is a transparent shim relaying everything to the
> underlay while overlayfs' upper layer is not (completely).

I get concerned any time shiftfs just copies up some non-trivial data
from the lower filesystem, that shiftfs is going to get bypassed and
some important metadata will not get propoerly updated in shiftfs. The
question that immediately comes to mind in this case is whether i_size
in the shiftfs indoe gets updated following an O_DIRECT write, and I
suspect tha there are other similar cases to worry about. Have you
considered this possibility and confirmed that no such issues exist?

Thanks,
Seth

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Christian Brauner-2
On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:

> On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1837223
> >
> > This enabled O_DIRECT support for shiftfs if the underlay supports it.
> >
> > Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> > This is blocking dqlite - an essential part of LXD - from profiting from
> > the performance benefits of O_DIRECT on suitable filesystems when used
> > with async io such as aio or io_uring.
> > Overlayfs cannot support this directly since the upper filesystem in
> > overlay can be any filesystem. So if the upper filesystem does not
> > support O_DIRECT but the lower filesystem does you're out of luck.
> > Shiftfs does not suffer from the same problem since there is not concept
> > of an upper filesystem in the same way that overlayfs has it.
> > Essentially, shiftfs is a transparent shim relaying everything to the
> > underlay while overlayfs' upper layer is not (completely).
>
> I get concerned any time shiftfs just copies up some non-trivial data
> from the lower filesystem, that shiftfs is going to get bypassed and
> some important metadata will not get propoerly updated in shiftfs. The
> question that immediately comes to mind in this case is whether i_size
> in the shiftfs indoe gets updated following an O_DIRECT write, and I
> suspect tha there are other similar cases to worry about. Have you

I'm not following, what are "other similar cases to worry about" and are
those already exposed?

Re: i_size for O_DIRECT
I have not seen prior art for filesystems that use iomap for O_DIRECT
that would fiddle with i_size so I didn't want to get into that game.
That is to say, shiftfs only supports O_DIRECT if the underlying
filesystem is iomap based. This was the same approach that overlayfs
wanted to use.

Christian

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Seth Forshee
On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:

> On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
> > On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> > > BugLink: https://bugs.launchpad.net/bugs/1837223
> > >
> > > This enabled O_DIRECT support for shiftfs if the underlay supports it.
> > >
> > > Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> > > This is blocking dqlite - an essential part of LXD - from profiting from
> > > the performance benefits of O_DIRECT on suitable filesystems when used
> > > with async io such as aio or io_uring.
> > > Overlayfs cannot support this directly since the upper filesystem in
> > > overlay can be any filesystem. So if the upper filesystem does not
> > > support O_DIRECT but the lower filesystem does you're out of luck.
> > > Shiftfs does not suffer from the same problem since there is not concept
> > > of an upper filesystem in the same way that overlayfs has it.
> > > Essentially, shiftfs is a transparent shim relaying everything to the
> > > underlay while overlayfs' upper layer is not (completely).
> >
> > I get concerned any time shiftfs just copies up some non-trivial data
> > from the lower filesystem, that shiftfs is going to get bypassed and
> > some important metadata will not get propoerly updated in shiftfs. The
> > question that immediately comes to mind in this case is whether i_size
> > in the shiftfs indoe gets updated following an O_DIRECT write, and I
> > suspect tha there are other similar cases to worry about. Have you
>
> I'm not following, what are "other similar cases to worry about" and are
> those already exposed?

I just mean anything else which might change in the underlay as a result
of I/O, e.g. timestamps.

> Re: i_size for O_DIRECT
> I have not seen prior art for filesystems that use iomap for O_DIRECT
> that would fiddle with i_size so I didn't want to get into that game.
> That is to say, shiftfs only supports O_DIRECT if the underlying
> filesystem is iomap based. This was the same approach that overlayfs
> wanted to use.

Are you saying that an O_DIRECT write that extends a file does not
result in i_size being updated? That doesn't sound right ...



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Christian Brauner-2
On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:

> On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
> > On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
> > > On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> > > > BugLink: https://bugs.launchpad.net/bugs/1837223
> > > >
> > > > This enabled O_DIRECT support for shiftfs if the underlay supports it.
> > > >
> > > > Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> > > > This is blocking dqlite - an essential part of LXD - from profiting from
> > > > the performance benefits of O_DIRECT on suitable filesystems when used
> > > > with async io such as aio or io_uring.
> > > > Overlayfs cannot support this directly since the upper filesystem in
> > > > overlay can be any filesystem. So if the upper filesystem does not
> > > > support O_DIRECT but the lower filesystem does you're out of luck.
> > > > Shiftfs does not suffer from the same problem since there is not concept
> > > > of an upper filesystem in the same way that overlayfs has it.
> > > > Essentially, shiftfs is a transparent shim relaying everything to the
> > > > underlay while overlayfs' upper layer is not (completely).
> > >
> > > I get concerned any time shiftfs just copies up some non-trivial data
> > > from the lower filesystem, that shiftfs is going to get bypassed and
> > > some important metadata will not get propoerly updated in shiftfs. The
> > > question that immediately comes to mind in this case is whether i_size
> > > in the shiftfs indoe gets updated following an O_DIRECT write, and I
> > > suspect tha there are other similar cases to worry about. Have you
> >
> > I'm not following, what are "other similar cases to worry about" and are
> > those already exposed?
>
> I just mean anything else which might change in the underlay as a result
> of I/O, e.g. timestamps.
>
> > Re: i_size for O_DIRECT
> > I have not seen prior art for filesystems that use iomap for O_DIRECT
> > that would fiddle with i_size so I didn't want to get into that game.
> > That is to say, shiftfs only supports O_DIRECT if the underlying
> > filesystem is iomap based. This was the same approach that overlayfs
> > wanted to use.
>
> Are you saying that an O_DIRECT write that extends a file does not
> result in i_size being updated? That doesn't sound right ...

For example, when overlayfs intended to introduce support for this
(which they didn't because of their version of upper and lower) they did
not mess with i_size. That's one of the reasons I didn't. Do you prefer
to copy up attributes to make sure we're not missing anything?

Christian

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Seth Forshee
On Fri, Jul 26, 2019 at 10:29:38AM +0200, Christian Brauner wrote:

> On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:
> > On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
> > > On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
> > > > On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> > > > > BugLink: https://bugs.launchpad.net/bugs/1837223
> > > > >
> > > > > This enabled O_DIRECT support for shiftfs if the underlay supports it.
> > > > >
> > > > > Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> > > > > This is blocking dqlite - an essential part of LXD - from profiting from
> > > > > the performance benefits of O_DIRECT on suitable filesystems when used
> > > > > with async io such as aio or io_uring.
> > > > > Overlayfs cannot support this directly since the upper filesystem in
> > > > > overlay can be any filesystem. So if the upper filesystem does not
> > > > > support O_DIRECT but the lower filesystem does you're out of luck.
> > > > > Shiftfs does not suffer from the same problem since there is not concept
> > > > > of an upper filesystem in the same way that overlayfs has it.
> > > > > Essentially, shiftfs is a transparent shim relaying everything to the
> > > > > underlay while overlayfs' upper layer is not (completely).
> > > >
> > > > I get concerned any time shiftfs just copies up some non-trivial data
> > > > from the lower filesystem, that shiftfs is going to get bypassed and
> > > > some important metadata will not get propoerly updated in shiftfs. The
> > > > question that immediately comes to mind in this case is whether i_size
> > > > in the shiftfs indoe gets updated following an O_DIRECT write, and I
> > > > suspect tha there are other similar cases to worry about. Have you
> > >
> > > I'm not following, what are "other similar cases to worry about" and are
> > > those already exposed?
> >
> > I just mean anything else which might change in the underlay as a result
> > of I/O, e.g. timestamps.
> >
> > > Re: i_size for O_DIRECT
> > > I have not seen prior art for filesystems that use iomap for O_DIRECT
> > > that would fiddle with i_size so I didn't want to get into that game.
> > > That is to say, shiftfs only supports O_DIRECT if the underlying
> > > filesystem is iomap based. This was the same approach that overlayfs
> > > wanted to use.
> >
> > Are you saying that an O_DIRECT write that extends a file does not
> > result in i_size being updated? That doesn't sound right ...
>
> For example, when overlayfs intended to introduce support for this
> (which they didn't because of their version of upper and lower) they did
> not mess with i_size. That's one of the reasons I didn't. Do you prefer
> to copy up attributes to make sure we're not missing anything?

I think that something like this:

  fd = open(path, O_RDWR|O_DIRECT);
  lseek(fd, 0, SEEK_END);
  write(fd, data, size);
  fstat(fd, &stat);
  printf("%ld\n", (long)stat.st_size);

Should print out the correct size (and similarly correct data for other
attributes). Maybe it will with this patch, I'm just not certain that it
will, so really I'm just asking whether you've tested this sort of
thing. If it doesn't work, then it seems like we probably do need to
copy up the attributes.

Seth

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Stefan Bader-2
On 26.07.19 14:59, Seth Forshee wrote:

> On Fri, Jul 26, 2019 at 10:29:38AM +0200, Christian Brauner wrote:
>> On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:
>>> On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
>>>> On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
>>>>> On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
>>>>>> BugLink: https://bugs.launchpad.net/bugs/1837223
>>>>>>
>>>>>> This enabled O_DIRECT support for shiftfs if the underlay supports it.
>>>>>>
>>>>>> Currently shiftfs does not handle O_DIRECT if the underlay supports it.
>>>>>> This is blocking dqlite - an essential part of LXD - from profiting from
>>>>>> the performance benefits of O_DIRECT on suitable filesystems when used
>>>>>> with async io such as aio or io_uring.
>>>>>> Overlayfs cannot support this directly since the upper filesystem in
>>>>>> overlay can be any filesystem. So if the upper filesystem does not
>>>>>> support O_DIRECT but the lower filesystem does you're out of luck.
>>>>>> Shiftfs does not suffer from the same problem since there is not concept
>>>>>> of an upper filesystem in the same way that overlayfs has it.
>>>>>> Essentially, shiftfs is a transparent shim relaying everything to the
>>>>>> underlay while overlayfs' upper layer is not (completely).
>>>>>
>>>>> I get concerned any time shiftfs just copies up some non-trivial data
>>>>> from the lower filesystem, that shiftfs is going to get bypassed and
>>>>> some important metadata will not get propoerly updated in shiftfs. The
>>>>> question that immediately comes to mind in this case is whether i_size
>>>>> in the shiftfs indoe gets updated following an O_DIRECT write, and I
>>>>> suspect tha there are other similar cases to worry about. Have you
>>>>
>>>> I'm not following, what are "other similar cases to worry about" and are
>>>> those already exposed?
>>>
>>> I just mean anything else which might change in the underlay as a result
>>> of I/O, e.g. timestamps.
>>>
>>>> Re: i_size for O_DIRECT
>>>> I have not seen prior art for filesystems that use iomap for O_DIRECT
>>>> that would fiddle with i_size so I didn't want to get into that game.
>>>> That is to say, shiftfs only supports O_DIRECT if the underlying
>>>> filesystem is iomap based. This was the same approach that overlayfs
>>>> wanted to use.
>>>
>>> Are you saying that an O_DIRECT write that extends a file does not
>>> result in i_size being updated? That doesn't sound right ...
>>
>> For example, when overlayfs intended to introduce support for this
>> (which they didn't because of their version of upper and lower) they did
>> not mess with i_size. That's one of the reasons I didn't. Do you prefer
>> to copy up attributes to make sure we're not missing anything?
>
> I think that something like this:
>
>   fd = open(path, O_RDWR|O_DIRECT);
>   lseek(fd, 0, SEEK_END);
>   write(fd, data, size);
>   fstat(fd, &stat);
>   printf("%ld\n", (long)stat.st_size);
>
> Should print out the correct size (and similarly correct data for other
> attributes). Maybe it will with this patch, I'm just not certain that it
> will, so really I'm just asking whether you've tested this sort of
> thing. If it doesn't work, then it seems like we probably do need to
> copy up the attributes.
>
> Seth
>
With the long discussion seemingly stopping unresolved I am not sure this
patchset is ready for SRU or not.

-Stefan


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Seth Forshee
On Mon, Aug 12, 2019 at 02:31:38PM +0200, Stefan Bader wrote:

> On 26.07.19 14:59, Seth Forshee wrote:
> > On Fri, Jul 26, 2019 at 10:29:38AM +0200, Christian Brauner wrote:
> >> On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:
> >>> On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
> >>>> On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
> >>>>> On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> >>>>>> BugLink: https://bugs.launchpad.net/bugs/1837223
> >>>>>>
> >>>>>> This enabled O_DIRECT support for shiftfs if the underlay supports it.
> >>>>>>
> >>>>>> Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> >>>>>> This is blocking dqlite - an essential part of LXD - from profiting from
> >>>>>> the performance benefits of O_DIRECT on suitable filesystems when used
> >>>>>> with async io such as aio or io_uring.
> >>>>>> Overlayfs cannot support this directly since the upper filesystem in
> >>>>>> overlay can be any filesystem. So if the upper filesystem does not
> >>>>>> support O_DIRECT but the lower filesystem does you're out of luck.
> >>>>>> Shiftfs does not suffer from the same problem since there is not concept
> >>>>>> of an upper filesystem in the same way that overlayfs has it.
> >>>>>> Essentially, shiftfs is a transparent shim relaying everything to the
> >>>>>> underlay while overlayfs' upper layer is not (completely).
> >>>>>
> >>>>> I get concerned any time shiftfs just copies up some non-trivial data
> >>>>> from the lower filesystem, that shiftfs is going to get bypassed and
> >>>>> some important metadata will not get propoerly updated in shiftfs. The
> >>>>> question that immediately comes to mind in this case is whether i_size
> >>>>> in the shiftfs indoe gets updated following an O_DIRECT write, and I
> >>>>> suspect tha there are other similar cases to worry about. Have you
> >>>>
> >>>> I'm not following, what are "other similar cases to worry about" and are
> >>>> those already exposed?
> >>>
> >>> I just mean anything else which might change in the underlay as a result
> >>> of I/O, e.g. timestamps.
> >>>
> >>>> Re: i_size for O_DIRECT
> >>>> I have not seen prior art for filesystems that use iomap for O_DIRECT
> >>>> that would fiddle with i_size so I didn't want to get into that game.
> >>>> That is to say, shiftfs only supports O_DIRECT if the underlying
> >>>> filesystem is iomap based. This was the same approach that overlayfs
> >>>> wanted to use.
> >>>
> >>> Are you saying that an O_DIRECT write that extends a file does not
> >>> result in i_size being updated? That doesn't sound right ...
> >>
> >> For example, when overlayfs intended to introduce support for this
> >> (which they didn't because of their version of upper and lower) they did
> >> not mess with i_size. That's one of the reasons I didn't. Do you prefer
> >> to copy up attributes to make sure we're not missing anything?
> >
> > I think that something like this:
> >
> >   fd = open(path, O_RDWR|O_DIRECT);
> >   lseek(fd, 0, SEEK_END);
> >   write(fd, data, size);
> >   fstat(fd, &stat);
> >   printf("%ld\n", (long)stat.st_size);
> >
> > Should print out the correct size (and similarly correct data for other
> > attributes). Maybe it will with this patch, I'm just not certain that it
> > will, so really I'm just asking whether you've tested this sort of
> > thing. If it doesn't work, then it seems like we probably do need to
> > copy up the attributes.
> >
> > Seth
> >
> With the long discussion seemingly stopping unresolved I am not sure this
> patchset is ready for SRU or not.

I think we are still waiting for Christian to get back with some test
results here, so these patches should wait.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Christian Brauner-2
After the sprint I was on vacation.
I'll try to get around to it soon.

Christian

On August 12, 2019 4:45:19 PM GMT+02:00, Seth Forshee <[hidden email]> wrote:
On Mon, Aug 12, 2019 at 02:31:38PM +0200, Stefan Bader wrote:
On 26.07.19 14:59, Seth Forshee wrote:
On Fri, Jul 26, 2019 at 10:29:38AM +0200, Christian Brauner wrote:
On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:
On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
BugLink: https://bugs.launchpad.net/bugs/1837223

This enabled O_DIRECT support for shiftfs if the underlay supports it.

Currently shiftfs does not handle O_DIRECT if the underlay supports it.
This is blocking dqlite - an essential part of LXD - from profiting from
the performance benefits of O_DIRECT on suitable filesystems when used
with async io such as aio or io_uring.
Overlayfs cannot support this directly since the upper filesystem in
overlay can be any filesystem. So if the upper filesystem does not
support O_DIRECT but the lower filesystem does you're out of luck.
Shiftfs does not suffer from the same problem since there is not concept
of an upper filesystem in the same way that overlayfs has it.
Essentially, shiftfs is a transparent shim relaying everything to the
underlay while overlayfs' upper layer is not (completely).

I get concerned any time shiftfs just copies up some non-trivial data
from the lower filesystem, that shiftfs is going to get bypassed and
some important metadata will not get propoerly updated in shiftfs. The
question that immediately comes to mind in this case is whether i_size
in the shiftfs indoe gets updated following an O_DIRECT write, and I
suspect tha there are other similar cases to worry about. Have you

I'm not following, what are "other similar cases to worry about" and are
those already exposed?

I just mean anything else which might change in the underlay as a result
of I/O, e.g. timestamps.

Re: i_size for O_DIRECT
I have not seen prior art for filesystems that use iomap for O_DIRECT
that would fiddle with i_size so I didn't want to get into that game.
That is to say, shiftfs only supports O_DIRECT if the underlying
filesystem is iomap based. This was the same approach that overlayfs
wanted to use.

Are you saying that an O_DIRECT write that extends a file does not
result in i_size being updated? That doesn't sound right ...

For example, when overlayfs intended to introduce support for this
(which they didn't because of their version of upper and lower) they did
not mess with i_size. That's one of the reasons I didn't. Do you prefer
to copy up attributes to make sure we're not missing anything?

I think that something like this:

fd = open(path, O_RDWR|O_DIRECT);
lseek(fd, 0, SEEK_END);
write(fd, data, size);
fstat(fd, &stat);
printf("%ld\n", (long)stat.st_size);

Should print out the correct size (and similarly correct data for other
attributes). Maybe it will with this patch, I'm just not certain that it
will, so really I'm just asking whether you've tested this sort of
thing. If it doesn't work, then it seems like we probably do need to
copy up the attributes.

Seth

With the long discussion seemingly stopping unresolved I am not sure this
patchset is ready for SRU or not.

I think we are still waiting for Christian to get back with some test
results here, so these patches should wait.

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2][SRU][Disco] UBUNTU: SAUCE: shiftfs: add O_DIRECT support

Christian Brauner-3
In reply to this post by Seth Forshee
On Mon, Aug 12, 2019 at 09:45:19AM -0500, Seth Forshee wrote:

> On Mon, Aug 12, 2019 at 02:31:38PM +0200, Stefan Bader wrote:
> > On 26.07.19 14:59, Seth Forshee wrote:
> > > On Fri, Jul 26, 2019 at 10:29:38AM +0200, Christian Brauner wrote:
> > >> On Thu, Jul 25, 2019 at 03:26:01PM -0500, Seth Forshee wrote:
> > >>> On Mon, Jul 22, 2019 at 05:05:08PM +0200, Christian Brauner wrote:
> > >>>> On Fri, Jul 19, 2019 at 03:02:21PM -0500, Seth Forshee wrote:
> > >>>>> On Fri, Jul 19, 2019 at 05:50:46PM +0200, Christian Brauner wrote:
> > >>>>>> BugLink: https://bugs.launchpad.net/bugs/1837223
> > >>>>>>
> > >>>>>> This enabled O_DIRECT support for shiftfs if the underlay supports it.
> > >>>>>>
> > >>>>>> Currently shiftfs does not handle O_DIRECT if the underlay supports it.
> > >>>>>> This is blocking dqlite - an essential part of LXD - from profiting from
> > >>>>>> the performance benefits of O_DIRECT on suitable filesystems when used
> > >>>>>> with async io such as aio or io_uring.
> > >>>>>> Overlayfs cannot support this directly since the upper filesystem in
> > >>>>>> overlay can be any filesystem. So if the upper filesystem does not
> > >>>>>> support O_DIRECT but the lower filesystem does you're out of luck.
> > >>>>>> Shiftfs does not suffer from the same problem since there is not concept
> > >>>>>> of an upper filesystem in the same way that overlayfs has it.
> > >>>>>> Essentially, shiftfs is a transparent shim relaying everything to the
> > >>>>>> underlay while overlayfs' upper layer is not (completely).
> > >>>>>
> > >>>>> I get concerned any time shiftfs just copies up some non-trivial data
> > >>>>> from the lower filesystem, that shiftfs is going to get bypassed and
> > >>>>> some important metadata will not get propoerly updated in shiftfs. The
> > >>>>> question that immediately comes to mind in this case is whether i_size
> > >>>>> in the shiftfs indoe gets updated following an O_DIRECT write, and I
> > >>>>> suspect tha there are other similar cases to worry about. Have you
> > >>>>
> > >>>> I'm not following, what are "other similar cases to worry about" and are
> > >>>> those already exposed?
> > >>>
> > >>> I just mean anything else which might change in the underlay as a result
> > >>> of I/O, e.g. timestamps.
> > >>>
> > >>>> Re: i_size for O_DIRECT
> > >>>> I have not seen prior art for filesystems that use iomap for O_DIRECT
> > >>>> that would fiddle with i_size so I didn't want to get into that game.
> > >>>> That is to say, shiftfs only supports O_DIRECT if the underlying
> > >>>> filesystem is iomap based. This was the same approach that overlayfs
> > >>>> wanted to use.
> > >>>
> > >>> Are you saying that an O_DIRECT write that extends a file does not
> > >>> result in i_size being updated? That doesn't sound right ...
> > >>
> > >> For example, when overlayfs intended to introduce support for this
> > >> (which they didn't because of their version of upper and lower) they did
> > >> not mess with i_size. That's one of the reasons I didn't. Do you prefer
> > >> to copy up attributes to make sure we're not missing anything?
> > >
> > > I think that something like this:
> > >
> > >   fd = open(path, O_RDWR|O_DIRECT);
> > >   lseek(fd, 0, SEEK_END);
> > >   write(fd, data, size);
> > >   fstat(fd, &stat);
> > >   printf("%ld\n", (long)stat.st_size);
> > >
> > > Should print out the correct size (and similarly correct data for other
> > > attributes). Maybe it will with this patch, I'm just not certain that it
> > > will, so really I'm just asking whether you've tested this sort of
> > > thing. If it doesn't work, then it seems like we probably do need to
> > > copy up the attributes.
> > >
> > > Seth
> > >
> > With the long discussion seemingly stopping unresolved I am not sure this
> > patchset is ready for SRU or not.
>
> I think we are still waiting for Christian to get back with some test
> results here, so these patches should wait.

The test outlined above shows that the correct file size is reported.

Christian

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/APPLIED: [PATCH 0/2][SRU][Disco] shiftfs: bugfix and O_DIRECT support

Seth Forshee
In reply to this post by Christian Brauner
On Fri, Jul 19, 2019 at 05:50:45PM +0200, Christian Brauner wrote:
> Hey everyone,
>
> Here's a tiny two-patch series that enables O_DIRECT support for shiftfs
> since this is needed for dqlite which is an essential component of LXD
> and fixes a bug whereby an unsigned long was passed to copy_from_user()
> instead of an void __user *p.
>
> Corresponding SRU justifications are up on Launchpad. The links to them
> can be found in the individual patches.

Acked-by: Seth Forshee <[hidden email]>

Applied to eoan/master-next and unstable/master, thanks!

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [PATCH 0/2][SRU][Disco] shiftfs: bugfix and O_DIRECT support

Stefan Bader-2
In reply to this post by Christian Brauner
On 19.07.19 17:50, Christian Brauner wrote:

> Hey everyone,
>
> Here's a tiny two-patch series that enables O_DIRECT support for shiftfs
> since this is needed for dqlite which is an essential component of LXD
> and fixes a bug whereby an unsigned long was passed to copy_from_user()
> instead of an void __user *p.
>
> Corresponding SRU justifications are up on Launchpad. The links to them
> can be found in the individual patches.
>
> Thanks!
> Christian
>
> Christian Brauner (2):
>   UBUNTU: SAUCE: shiftfs: add O_DIRECT support
>   UBUNTU: SAUCE: shiftfs: pass correct point down
>
>  fs/shiftfs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

APPLIED: [PATCH 0/2][SRU][Disco] shiftfs: bugfix and O_DIRECT support

Kleber Souza
In reply to this post by Christian Brauner
On 7/19/19 5:50 PM, Christian Brauner wrote:

> Hey everyone,
>
> Here's a tiny two-patch series that enables O_DIRECT support for shiftfs
> since this is needed for dqlite which is an essential component of LXD
> and fixes a bug whereby an unsigned long was passed to copy_from_user()
> instead of an void __user *p.
>
> Corresponding SRU justifications are up on Launchpad. The links to them
> can be found in the individual patches.
>
> Thanks!
> Christian
>
> Christian Brauner (2):
>   UBUNTU: SAUCE: shiftfs: add O_DIRECT support
>   UBUNTU: SAUCE: shiftfs: pass correct point down
>
>  fs/shiftfs.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>

Applied to disco/master-next branch.

Thanks,
Kleber


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team