| Message ID | 20191025095849.25283-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/file-posix: Work around XFS bug | expand |
Am 25.10.2019 um 11:58 hat Max Reitz geschrieben: > We will need this for the next patch. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/file-posix.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 695fcf740d..5cd54c8bff 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -149,7 +149,7 @@ typedef struct BDRVRawState { > int perm_change_flags; > BDRVReopenState *reopen_state; > > -#ifdef CONFIG_XFS > +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > bool is_xfs:1; > #endif > bool has_discard:1; > @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > } > #endif > > -#ifdef CONFIG_XFS > +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > if (platform_test_xfs_fd(s->fd)) { platform_test_xfs_fd() is defined in a header file from xfsprogs. I don't think you can call that without CONFIG_XFS, it would break the build. > s->is_xfs = true; > } Kevin
On 25.10.19 12:19, Kevin Wolf wrote: > Am 25.10.2019 um 11:58 hat Max Reitz geschrieben: >> We will need this for the next patch. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/file-posix.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 695fcf740d..5cd54c8bff 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -149,7 +149,7 @@ typedef struct BDRVRawState { >> int perm_change_flags; >> BDRVReopenState *reopen_state; >> >> -#ifdef CONFIG_XFS >> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) >> bool is_xfs:1; >> #endif >> bool has_discard:1; >> @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >> } >> #endif >> >> -#ifdef CONFIG_XFS >> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) >> if (platform_test_xfs_fd(s->fd)) { > > platform_test_xfs_fd() is defined in a header file from xfsprogs. I > don't think you can call that without CONFIG_XFS, it would break the > build. Aw. OK. Should we then just assume is_xfs to be true (for the next patch) with !defined(CONFIG_XFS) && defined(CONFIG_FALLOCATE)? Max
Am 25.10.2019 um 12:22 hat Max Reitz geschrieben: > On 25.10.19 12:19, Kevin Wolf wrote: > > Am 25.10.2019 um 11:58 hat Max Reitz geschrieben: > >> We will need this for the next patch. > >> > >> Signed-off-by: Max Reitz <mreitz@redhat.com> > >> --- > >> block/file-posix.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/block/file-posix.c b/block/file-posix.c > >> index 695fcf740d..5cd54c8bff 100644 > >> --- a/block/file-posix.c > >> +++ b/block/file-posix.c > >> @@ -149,7 +149,7 @@ typedef struct BDRVRawState { > >> int perm_change_flags; > >> BDRVReopenState *reopen_state; > >> > >> -#ifdef CONFIG_XFS > >> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > >> bool is_xfs:1; > >> #endif > >> bool has_discard:1; > >> @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > >> } > >> #endif > >> > >> -#ifdef CONFIG_XFS > >> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > >> if (platform_test_xfs_fd(s->fd)) { > > > > platform_test_xfs_fd() is defined in a header file from xfsprogs. I > > don't think you can call that without CONFIG_XFS, it would break the > > build. > > Aw. > > OK. Should we then just assume is_xfs to be true (for the next patch) > with !defined(CONFIG_XFS) && defined(CONFIG_FALLOCATE)? It's a small inline function that basically just calls statfs() and then checks f_type. I think we can have a small function to implement this in the file-posix code. Don't copy it from the header because of licensing (LGPL, while file-posix is MIT); refer to the statfs() manpage instead and write it yourself. Kevin
On 25.10.19 12:35, Kevin Wolf wrote: > Am 25.10.2019 um 12:22 hat Max Reitz geschrieben: >> On 25.10.19 12:19, Kevin Wolf wrote: >>> Am 25.10.2019 um 11:58 hat Max Reitz geschrieben: >>>> We will need this for the next patch. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> block/file-posix.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/block/file-posix.c b/block/file-posix.c >>>> index 695fcf740d..5cd54c8bff 100644 >>>> --- a/block/file-posix.c >>>> +++ b/block/file-posix.c >>>> @@ -149,7 +149,7 @@ typedef struct BDRVRawState { >>>> int perm_change_flags; >>>> BDRVReopenState *reopen_state; >>>> >>>> -#ifdef CONFIG_XFS >>>> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) >>>> bool is_xfs:1; >>>> #endif >>>> bool has_discard:1; >>>> @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, >>>> } >>>> #endif >>>> >>>> -#ifdef CONFIG_XFS >>>> +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) >>>> if (platform_test_xfs_fd(s->fd)) { >>> >>> platform_test_xfs_fd() is defined in a header file from xfsprogs. I >>> don't think you can call that without CONFIG_XFS, it would break the >>> build. >> >> Aw. >> >> OK. Should we then just assume is_xfs to be true (for the next patch) >> with !defined(CONFIG_XFS) && defined(CONFIG_FALLOCATE)? > > It's a small inline function that basically just calls statfs() and then > checks f_type. Yes, this is where my error came from. I asked cscope, which happily referred me to the inline implementation and so I didn’t bother looking at the filename and just assumed it’d be some code that belongs to qemu. > I think we can have a small function to implement this in the file-posix > code. Don't copy it from the header because of licensing (LGPL, while > file-posix is MIT); refer to the statfs() manpage instead and write it > yourself. OK. Max
On Fri, Oct 25, 2019 at 1:22 PM Max Reitz <mreitz@redhat.com> wrote: > > We will need this for the next patch. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/file-posix.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 695fcf740d..5cd54c8bff 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -149,7 +149,7 @@ typedef struct BDRVRawState { > int perm_change_flags; > BDRVReopenState *reopen_state; > > -#ifdef CONFIG_XFS > +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > bool is_xfs:1; > #endif > bool has_discard:1; > @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > } > #endif > > -#ifdef CONFIG_XFS > +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) > if (platform_test_xfs_fd(s->fd)) { > s->is_xfs = true; What about remote xfs filesystem, e.g. glusterfs over xfs mounted using fuse? (how oVirt uses glusterfs) The buggy behavior with concurrent fallocate/pwrite can affect this, and platform_test_xfs_fd() will probably fail to detect xfs. Nir > } > -- > 2.21.0 > >
diff --git a/block/file-posix.c b/block/file-posix.c index 695fcf740d..5cd54c8bff 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -149,7 +149,7 @@ typedef struct BDRVRawState { int perm_change_flags; BDRVReopenState *reopen_state; -#ifdef CONFIG_XFS +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) bool is_xfs:1; #endif bool has_discard:1; @@ -667,7 +667,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, } #endif -#ifdef CONFIG_XFS +#if defined(CONFIG_XFS) || defined(CONFIG_FALLOCATE) if (platform_test_xfs_fd(s->fd)) { s->is_xfs = true; }
We will need this for the next patch. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/file-posix.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)