9:38 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1077068
The virsh vol-info (and virsh domblkinfo) commands return an incorrect
size if the target device is an NFS volume. These patches will add a
check for the target being NFS and rather than calculate the size based
on "st_blocks" and "DEV_BSIZE" (512) use the "st_size" value
instead.
John Ferlan (4):
virfile: Split virFileGetFSFtype
virfile: Introduce virFileIsNFSFSType
vol-info: Check for NFS FS type
domblkinfo: Check for NFS FS Type
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 7 ++++--
src/storage/storage_backend.c | 7 ++++--
src/util/virfile.c | 53 +++++++++++++++++++++++++++++++++++--------
src/util/virfile.h | 1 +
5 files changed, 55 insertions(+), 14 deletions(-)
--
1.9.3
9:38 a.m.
New subject: [libvirt] [PATCH 1/4] virfile: Split virFileGetFSFtype
Split out the portion of virFileGetFSFtype that makes the statfs call
in order to return the "f_type" field
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/util/virfile.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 9863fd0..7612007 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2841,14 +2841,16 @@ int virFilePrintf(FILE *fp, const char *msg, ...)
# define HUGETLBFS_MAGIC 0x958458f6
# endif
-int
-virFileIsSharedFSType(const char *path,
- int fstypes)
+static int
+virFileGetFSFtype(const char *path,
+ long long int *ret_ftype)
{
char *dirpath, *p;
struct statfs sb;
int statfs_ret;
+ *ret_ftype = 0;
+
if (VIR_STRDUP(dirpath, path) < 0)
return -1;
@@ -2886,28 +2888,40 @@ virFileIsSharedFSType(const char *path,
path);
return -1;
}
+ *ret_ftype = sb.f_type;
+ return 0;
+}
+
+int
+virFileIsSharedFSType(const char *path,
+ int fstypes)
+{
+ long long int f_type;
+
+ if (virFileGetFSFtype(path, &f_type) < 0)
+ return -1;
VIR_DEBUG("Check if path %s with FS magic %lld is shared",
- path, (long long int)sb.f_type);
+ path, f_type);
if ((fstypes & VIR_FILE_SHFS_NFS) &&
- (sb.f_type == NFS_SUPER_MAGIC))
+ (f_type == NFS_SUPER_MAGIC))
return 1;
if ((fstypes & VIR_FILE_SHFS_GFS2) &&
- (sb.f_type == GFS2_MAGIC))
+ (f_type == GFS2_MAGIC))
return 1;
if ((fstypes & VIR_FILE_SHFS_OCFS) &&
- (sb.f_type == OCFS2_SUPER_MAGIC))
+ (f_type == OCFS2_SUPER_MAGIC))
return 1;
if ((fstypes & VIR_FILE_SHFS_AFS) &&
- (sb.f_type == AFS_FS_MAGIC))
+ (f_type == AFS_FS_MAGIC))
return 1;
if ((fstypes & VIR_FILE_SHFS_SMB) &&
- (sb.f_type == SMB_SUPER_MAGIC))
+ (f_type == SMB_SUPER_MAGIC))
return 1;
if ((fstypes & VIR_FILE_SHFS_CIFS) &&
- (sb.f_type == CIFS_SUPER_MAGIC))
+ (f_type == CIFS_SUPER_MAGIC))
return 1;
return 0;
--
1.9.3
9:38 a.m.
New subject: [libvirt] [PATCH 2/4] virfile: Introduce virFileIsNFSFSType
Use the virFileGetFSFtype() in order to compare the returned
f_type for the NFS super magic number
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 19 +++++++++++++++++++
src/util/virfile.h | 1 +
3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 08111d4..121e578 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1291,6 +1291,7 @@ virFileIsDir;
virFileIsExecutable;
virFileIsLink;
virFileIsMountPoint;
+virFileIsNFSFSType;
virFileIsSharedFS;
virFileIsSharedFSType;
virFileLinkPointsTo;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 7612007..e6c767d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2927,6 +2927,18 @@ virFileIsSharedFSType(const char *path,
return 0;
}
+bool
+virFileIsNFSFSType(const char *path)
+{
+ long long int f_type;
+
+ if ((virFileGetFSFtype(path, &f_type) == 0) &&
+ (f_type == NFS_SUPER_MAGIC))
+ return true;
+
+ return false;
+}
+
int
virFileGetHugepageSize(const char *path,
unsigned long long *size)
@@ -3060,6 +3072,13 @@ int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED,
}
int
+virFileIsNFSFSType(const char *path ATTRIBUTE_UNUSED)
+{
+ /* XXX implement me :-) */
+ return false;
+}
+
+int
virFileGetHugepageSize(const char *path ATTRIBUTE_UNUSED,
unsigned long long *size ATTRIBUTE_UNUSED)
{
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 403d0ba..cc07c53 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -191,6 +191,7 @@ enum {
};
int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
+bool virFileIsNFSFSType(const char *path);
int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1);
int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
--
1.9.3
6:07 a.m.
New subject: [libvirt] [PATCH 2/4] virfile: Introduce virFileIsNFSFSType
On 08/05/2014 04:38 PM, John Ferlan wrote:
Use the virFileGetFSFtype() in order to compare the returned
f_type for the NFS super magic number
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virfile.c | 19 +++++++++++++++++++
src/util/virfile.h | 1 +
3 files changed, 21 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 08111d4..121e578 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1291,6 +1291,7 @@ virFileIsDir;
virFileIsExecutable;
virFileIsLink;
virFileIsMountPoint;
+virFileIsNFSFSType;
virFileIsSharedFS;
virFileIsSharedFSType;
virFileLinkPointsTo;
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 7612007..e6c767d 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2927,6 +2927,18 @@ virFileIsSharedFSType(const char *path,
return 0;
}
+bool
+virFileIsNFSFSType(const char *path)
+{
+ long long int f_type;
+
+ if ((virFileGetFSFtype(path, &f_type) == 0) &&
+ (f_type == NFS_SUPER_MAGIC))
+ return true;
+
+ return false;
+}
+
int
virFileGetHugepageSize(const char *path,
unsigned long long *size)
@@ -3060,6 +3072,13 @@ int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED,
}
int
+virFileIsNFSFSType(const char *path ATTRIBUTE_UNUSED)
This doesn't match the prototype in virfile.h.
Also, I wonder if virFileIsNFSFSType(path) is that much more readable than
virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 1.
+{
+ /* XXX implement me :-) */
+ return false;
+}
+
+int
virFileGetHugepageSize(const char *path ATTRIBUTE_UNUSED,
unsigned long long *size ATTRIBUTE_UNUSED)
{
diff --git a/src/util/virfile.h b/src/util/virfile.h
index 403d0ba..cc07c53 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -191,6 +191,7 @@ enum {
};
int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
+bool virFileIsNFSFSType(const char *path);
ATTRIBUTE_NONNULL(1);
int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1);
int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
ACK
6:42 a.m.
New subject: [libvirt] [PATCH 2/4] virfile: Introduce virFileIsNFSFSType
On 08/08/2014 07:07 AM, Ján Tomko wrote:
On 08/05/2014 04:38 PM, John Ferlan wrote:
> Use the virFileGetFSFtype() in order to compare the returned
> f_type for the NFS super magic number
>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> src/libvirt_private.syms | 1 +
> src/util/virfile.c | 19 +++++++++++++++++++
> src/util/virfile.h | 1 +
> 3 files changed, 21 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index 08111d4..121e578 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1291,6 +1291,7 @@ virFileIsDir;
> virFileIsExecutable;
> virFileIsLink;
> virFileIsMountPoint;
> +virFileIsNFSFSType;
> virFileIsSharedFS;
> virFileIsSharedFSType;
> virFileLinkPointsTo;
> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index 7612007..e6c767d 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2927,6 +2927,18 @@ virFileIsSharedFSType(const char *path,
> return 0;
> }
>
> +bool
> +virFileIsNFSFSType(const char *path)
> +{
> + long long int f_type;
> +
> + if ((virFileGetFSFtype(path, &f_type) == 0) &&
> + (f_type == NFS_SUPER_MAGIC))
> + return true;
> +
> + return false;
> +}
> +
> int
> virFileGetHugepageSize(const char *path,
> unsigned long long *size)
> @@ -3060,6 +3072,13 @@ int virFileIsSharedFSType(const char *path ATTRIBUTE_UNUSED,
> }
>
> int
> +virFileIsNFSFSType(const char *path ATTRIBUTE_UNUSED)
This doesn't match the prototype in virfile.h.
Drat - boy I hate making the same mistake twice - changed the function
late, but forgot to change the #else...
Also, I wonder if virFileIsNFSFSType(path) is that much more readable
than
virFileIsSharedFSType(path, VIR_FILE_SHFS_NFS) == 1.
hmm... guess that makes patch 1 & 2 unnecessary. Guess I was more
hyperfocused on how to get at NFS_SUPER_MAGIC that I missed the SHFS enum
I'll rework...
John
> +{
> + /* XXX implement me :-) */
> + return false;
> +}
> +
> +int
> virFileGetHugepageSize(const char *path ATTRIBUTE_UNUSED,
> unsigned long long *size ATTRIBUTE_UNUSED)
> {
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index 403d0ba..cc07c53 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -191,6 +191,7 @@ enum {
> };
>
> int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1);
> +bool virFileIsNFSFSType(const char *path);
ATTRIBUTE_NONNULL(1);
> int virFileIsSharedFS(const char *path) ATTRIBUTE_NONNULL(1);
> int virFileIsMountPoint(const char *file) ATTRIBUTE_NONNULL(1);
>
>
ACK
9:38 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
https://bugzilla.redhat.com/show_bug.cgi?id=1077068
Check for the NFS FS type being true for a "local" stat of the file
to force usage of the 'st_size' value rather than calculating the size
using the 'st_blocks' and 'st_blksize'. As described in the stat(2)
man page:
"Use of the st_blocks and st_blksize fields may be less portable."
experimentation shows a 10M file could get the following output from stat:
st_size=10485760 st_blocks=88 st_blksize=1048576
resulting in a "44 KiB" value being displayed as the allocation value.
While this value does match the "du -s" value of the same file, the
"du -b" value shows the "st_size" field. Similarly a long listing of
the
file shows the 10M size.
Prior to the change:
Name: test-vol1
Type: file
Capacity: 10.00 MiB
Allocation: 44.00 KiB
After the change:
Name: test-vol1
Type: file
Capacity: 10.00 MiB
Allocation: 10.00 MiB
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/storage/storage_backend.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 5cada39..bbc992e 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -1520,8 +1520,11 @@ virStorageBackendUpdateVolTargetInfoFD(virStorageSourcePtr target,
if (S_ISREG(sb->st_mode)) {
#ifndef WIN32
- target->allocation = (unsigned long long)sb->st_blocks *
- (unsigned long long)DEV_BSIZE;
+ if (virFileIsNFSFSType(target->path))
+ target->allocation = sb->st_size;
+ else
+ target->allocation = (unsigned long long)sb->st_blocks *
+ (unsigned long long)DEV_BSIZE;
#else
target->allocation = sb->st_size;
#endif
--
1.9.3
6:07 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
On 08/05/2014 04:38 PM, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1077068
Check for the NFS FS type being true for a "local" stat of the file
to force usage of the 'st_size' value rather than calculating the size
using the 'st_blocks' and 'st_blksize'. As described in the stat(2)
man page:
"Use of the st_blocks and st_blksize fields may be less portable."
experimentation shows a 10M file could get the following output from stat:
st_size=10485760 st_blocks=88 st_blksize=1048576
resulting in a "44 KiB" value being displayed as the allocation value.
While this value does match the "du -s" value of the same file, the
"du -b" value shows the "st_size" field. Similarly a long listing of
the
file shows the 10M size.
Capacity should be the apparent size (what du -b shows, or st_size), while
allocation should track the on-disk usage (du, st_blocks * 512).
It looks to me that the values are correct, it's just that posix_fallocate
does neither work nor fail on NFS.
Jan
6:57 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
On 08/08/2014 07:07 AM, Ján Tomko wrote:
On 08/05/2014 04:38 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1077068
>
> Check for the NFS FS type being true for a "local" stat of the file
> to force usage of the 'st_size' value rather than calculating the size
> using the 'st_blocks' and 'st_blksize'. As described in the stat(2)
> man page:
>
> "Use of the st_blocks and st_blksize fields may be less portable."
>
> experimentation shows a 10M file could get the following output from stat:
>
> st_size=10485760 st_blocks=88 st_blksize=1048576
>
> resulting in a "44 KiB" value being displayed as the allocation value.
> While this value does match the "du -s" value of the same file, the
> "du -b" value shows the "st_size" field. Similarly a long listing
of the
> file shows the 10M size.
Capacity should be the apparent size (what du -b shows, or st_size), while
allocation should track the on-disk usage (du, st_blocks * 512).
It looks to me that the values are correct, it's just that posix_fallocate
does neither work nor fail on NFS.
Ah yes - this is exactly where I went back and forth on... Digging
through 'old' google results on posix_fallocate() and wondering whether
it was incorrectly returning success or whether stat() was as pointed
out in its man page not getting a reliable st_blocks value.
However, if posix_fallocate() isn't working as specified for NFS and not
producing any error message, then how does one really determine that?
I also had some code that reworked the two callers/users in order to
force the "allocation" paths to go through the slower lseek/safewrite
calls. Is it worth resurrecting that and going with it?
John
10:26 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
On 08/08/2014 07:07 AM, Ján Tomko wrote:
On 08/05/2014 04:38 PM, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1077068
>
> Check for the NFS FS type being true for a "local" stat of the file
> to force usage of the 'st_size' value rather than calculating the size
> using the 'st_blocks' and 'st_blksize'. As described in the stat(2)
> man page:
>
> "Use of the st_blocks and st_blksize fields may be less portable."
>
> experimentation shows a 10M file could get the following output from stat:
>
> st_size=10485760 st_blocks=88 st_blksize=1048576
>
> resulting in a "44 KiB" value being displayed as the allocation value.
> While this value does match the "du -s" value of the same file, the
> "du -b" value shows the "st_size" field. Similarly a long listing
of the
> file shows the 10M size.
Capacity should be the apparent size (what du -b shows, or st_size), while
allocation should track the on-disk usage (du, st_blocks * 512).
It looks to me that the values are correct, it's just that posix_fallocate
does neither work nor fail on NFS.
Jan
Testing seems to indicate that posix_fallocate() either doesn't work as
expected on the target or using the target.path is incorrect...
Before posix_fallocate
stat st_blocks=0 st_blksize=1048576 st_size=10485760
lseek end=10485760
posix_fallocate of 10485760 bytes on /home/nfs_pool/target/test-vol1
After posix_fallocate
stat st_blocks=88 st_blksize=1048576 st_size=10485760
lseek end=10485760
Hmm... would going at the target be correct in this instance? Same test
but use the source path:
Before posix_fallocate
stat st_blocks=0 st_blksize=4096 st_size=10485760
lseek end=10485760
posix_fallocate of 10485760 bytes on /home/nfs_pool/nfs-export/test-vol1
After posix_fallocate
stat st_blocks=20480 st_blksize=4096 st_size=10485760
lseek end=10485760
...
hmm.... 20480 * 512 = 10485760
# df
...
localhost:/home/nfs_pool/nfs-export 140979200 35521536 98273280 27%
/home/nfs_pool/target
#
John
5:28 p.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
On 08/08/2014 11:26 AM, John Ferlan wrote:
On 08/08/2014 07:07 AM, Ján Tomko wrote:
> On 08/05/2014 04:38 PM, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1077068
>>
>> Check for the NFS FS type being true for a "local" stat of the file
>> to force usage of the 'st_size' value rather than calculating the size
>> using the 'st_blocks' and 'st_blksize'. As described in the
stat(2)
>> man page:
>>
>> "Use of the st_blocks and st_blksize fields may be less portable."
>>
>> experimentation shows a 10M file could get the following output from stat:
>>
>> st_size=10485760 st_blocks=88 st_blksize=1048576
>>
>> resulting in a "44 KiB" value being displayed as the allocation value.
>> While this value does match the "du -s" value of the same file, the
>> "du -b" value shows the "st_size" field. Similarly a long
listing of the
>> file shows the 10M size.
>
> Capacity should be the apparent size (what du -b shows, or st_size), while
> allocation should track the on-disk usage (du, st_blocks * 512).
>
> It looks to me that the values are correct, it's just that posix_fallocate
> does neither work nor fail on NFS.
>
> Jan
>
Testing seems to indicate that posix_fallocate() either doesn't work as
expected on the target or using the target.path is incorrect...
Before posix_fallocate
stat st_blocks=0 st_blksize=1048576 st_size=10485760
lseek end=10485760
posix_fallocate of 10485760 bytes on /home/nfs_pool/target/test-vol1
After posix_fallocate
stat st_blocks=88 st_blksize=1048576 st_size=10485760
lseek end=10485760
Hmm... would going at the target be correct in this instance? Same test
but use the source path:
Before posix_fallocate
stat st_blocks=0 st_blksize=4096 st_size=10485760
lseek end=10485760
posix_fallocate of 10485760 bytes on /home/nfs_pool/nfs-export/test-vol1
After posix_fallocate
stat st_blocks=20480 st_blksize=4096 st_size=10485760
lseek end=10485760
...
hmm.... 20480 * 512 = 10485760
# df
...
localhost:/home/nfs_pool/nfs-export 140979200 35521536 98273280 27%
/home/nfs_pool/target
#
Well it's a tangled web that's being weaved... The blksize of the
target volume comes from the 'wsize' value in the mount:
localhost:/home/nfs_pool/nfs-export on /home/nfs_pool/target type nfs4
(rw,relatime,vers=4.0,rsize=1048576,wsize=1048576,...)
Further testing shows if I change the wsize to 4096, then I get what I
expect; however, starting at 8192 I'll get decreasingly smaller
allocations. So this is a "math problem".
So going back to writing via posix_fallocate() to the
"/home/nfs_pool/target/test-vol" the "issue" is the blksize of the
"source" (nfs-export) is 4096 while the blksize of the "target"
(target)
is 1048576 (as a result of the nfs mount settings). What "seems" to
happen is the posix_fallocate() makes 11 "writes" - I assume because
blksize*10 < desired_size (10485760) (instead of <=...).
Thus 11 * 4096 = 45056 (bytes) / 1024 = 44 KiB which was displayed.
Why all this happens I'm not sure. Bug in posix_fallocate()? Bug in
configuration? I have to assume that when this code was first added NFS
probably was still using smaller block sizes. Whether anyone has
noticed or not beyond the virt-test which discovered the issue - I'm not
sure. In any case, does anyone have feedback/thoughts for next steps?
I can put together something that avoids posix_fallocate() for the
create-as and resize paths.
John
7:54 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
On 08/09/2014 12:28 AM, John Ferlan wrote:
> Testing seems to indicate that posix_fallocate() either
doesn't work as
> expected on the target or using the target.path is incorrect...
>
> Before posix_fallocate
> stat st_blocks=0 st_blksize=1048576 st_size=10485760
> lseek end=10485760
>
> posix_fallocate of 10485760 bytes on /home/nfs_pool/target/test-vol1
>
> After posix_fallocate
> stat st_blocks=88 st_blksize=1048576 st_size=10485760
> lseek end=10485760
>
>
> Hmm... would going at the target be correct in this instance? Same test
> but use the source path:
Using the source path would only work if the NFS export is on the localhost :)
>
> Before posix_fallocate
> stat st_blocks=0 st_blksize=4096 st_size=10485760
> lseek end=10485760
>
> posix_fallocate of 10485760 bytes on /home/nfs_pool/nfs-export/test-vol1
>
> After posix_fallocate
> stat st_blocks=20480 st_blksize=4096 st_size=10485760
> lseek end=10485760
>
> ...
>
> hmm.... 20480 * 512 = 10485760
>
> # df
> ...
> localhost:/home/nfs_pool/nfs-export 140979200 35521536 98273280 27%
> /home/nfs_pool/target
> #
>
Well it's a tangled web that's being weaved... The blksize of the
target volume comes from the 'wsize' value in the mount:
localhost:/home/nfs_pool/nfs-export on /home/nfs_pool/target type nfs4
(rw,relatime,vers=4.0,rsize=1048576,wsize=1048576,...)
Further testing shows if I change the wsize to 4096, then I get what I
expect; however, starting at 8192 I'll get decreasingly smaller
allocations. So this is a "math problem".
So going back to writing via posix_fallocate() to the
"/home/nfs_pool/target/test-vol" the "issue" is the blksize of the
"source" (nfs-export) is 4096 while the blksize of the "target"
(target)
is 1048576 (as a result of the nfs mount settings). What "seems" to
happen is the posix_fallocate() makes 11 "writes" - I assume because
blksize*10 < desired_size (10485760) (instead of <=...).
Thus 11 * 4096 = 45056 (bytes) / 1024 = 44 KiB which was displayed.
Why all this happens I'm not sure. Bug in posix_fallocate()? Bug in
configuration? I have to assume that when this code was first added NFS
probably was still using smaller block sizes.
The code was introduced in 2013. Maybe it wasn't tested on NFS at all?
It looks like a posix_fallocate bug to me.
The other method we have (syscall(SYS_fallocate)) gives me EOPNOTSUPP -
Operation not supported on transport endpoint.
Whether anyone has
noticed or not beyond the virt-test which discovered the issue - I'm not
sure. In any case, does anyone have feedback/thoughts for next steps?
I can put together something that avoids posix_fallocate() for the
create-as and resize paths.
I think reporting an error when preallocate is requested for a NFS pool makes
sense, but that might be pretty annoying if something supplies the preallocate
flag by default.
Jan
8:47 a.m.
New subject: [libvirt] [PATCH 3/4] vol-info: Check for NFS FS type
<...snip...>
> Why all this happens I'm not sure. Bug in posix_fallocate()?
Bug in
> configuration? I have to assume that when this code was first added NFS
> probably was still using smaller block sizes.
The code was introduced in 2013. Maybe it wasn't tested on NFS at all?
It looks like a posix_fallocate bug to me.
Technically it was before that - the safezero function and it's
supporting cast were moved into virfile.c in 2013; however, they were in
[vir]util.c before that. It was first introduced in 2009 in commit id
'c29d0929' and beyond some minor changes/reword has stayed relatively
unchanged.
The one downside I see with it is that it's a build related either/or
setup. That is the "HAVE_POSIX_FALLOCATE" dictates the usage and
provides no fallback in the event that perhaps the result isn't expected
or if for some reason posix_fallocate fails.
"Theoretically" the safewrite option should/could be the fallback in the
event it's determined that the posix_fallocate() didn't generate the
right allocation size.
The other method we have (syscall(SYS_fallocate)) gives me EOPNOTSUPP
-
Operation not supported on transport endpoint.
Right - I saw that too.... The resize code followed the safezero() code
at least with respect to the either/or with the HAVE_POSIX_FALLOCATE
build conditional. It was introduced in commit id 'aa2a4cff'. It
assumes that the "SYS_fallocate" syscall is present "and" will work.
It
is missing an option to safewrite() from current allocation to the new
end of the file (eg, a call to safezero passing the the current offset).
> Whether anyone has
> noticed or not beyond the virt-test which discovered the issue - I'm not
> sure. In any case, does anyone have feedback/thoughts for next steps?
> I can put together something that avoids posix_fallocate() for the
> create-as and resize paths.
>
I think reporting an error when preallocate is requested for a NFS pool makes
sense, but that might be pretty annoying if something supplies the preallocate
flag by default.
My comment was more geared towards my findings of the posix_fallocate()
introduction in 2009.
While I agree there's something flaky with posix_fallocate, I don't
think an error is necessarily the best path. Maybe better "fall back
code" - we could check the posix_fallocate() results (using stat)
perhaps VIR_WARN that something is wrong before falling back to the
mmap/safewrite options.
John
9:39 a.m.
New subject: [libvirt] [PATCH 4/4] domblkinfo: Check for NFS FS Type
Similar to the vol-info code, check if the target device for the
blkinfo fetch is an NFS volume and return the st_size value rather than
calculating the size based on st_blksize and DEV_BSIZE.
Prior to the change a 40M target device would display:
Capacity: 41943040
Allocation: 126976
Physical: 126976
With the change it will display:
Capacity: 41943040
Allocation: 41943040
Physical: 41943040
Signed-off-by: John Ferlan <jferlan(a)redhat.com>
---
src/qemu/qemu_driver.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 8eba8e8..f5d31f6 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10432,8 +10432,11 @@ qemuDomainGetBlockInfo(virDomainPtr dom,
/* Get info for normal formats */
if (S_ISREG(sb.st_mode) || fd == -1) {
#ifndef WIN32
- info->physical = (unsigned long long)sb.st_blocks *
- (unsigned long long)DEV_BSIZE;
+ if (virFileIsNFSFSType(disk->src->path))
+ info->physical = sb.st_size;
+ else
+ info->physical = (unsigned long long)sb.st_blocks *
+ (unsigned long long)DEV_BSIZE;
#else
info->physical = sb.st_size;
#endif
--
1.9.3
3756
days inactive
3762
days old
12 comments
2 participants
participants (2)
-
John Ferlan -
Ján Tomko