public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print
@ 2024-09-24 14:33 Filip Schauer
  2024-09-25  8:39 ` Dietmar Maurer
  2024-09-25 14:37 ` Dominik Csapak
  0 siblings, 2 replies; 5+ messages in thread
From: Filip Schauer @ 2024-09-24 14:33 UTC (permalink / raw)
  To: pve-devel

The use of `print` can be inefficient for writing larger files due to
its default buffering in 8 KiB blocks.

This is especially problematic on `pmxcfs` where files are written in
4 KiB blocks due to the defaults of `libfuse2`. This leads to
significant write amplification on files larger than 4 KiB.

Patch (fix #5728: pmxcfs: allow bigger writes than 4k for fuse) [1]
addresses this by enabling `big_writes`, allowing up to 128 KiB blocks.
But due to the use of `print` in `file_set_contents`, writes are still
only buffered in 8 KiB blocks.

To further address this, this commit switches to using `syswrite`
instead of `print` to mitigate the block size limit imposed by `print`.
Combined with patch [1], file writes to `/etc/pve/` are now buffered in
128 KiB blocks.

The table below illustrates the drastic reduction in write
amplification when writing files of different sizes to `/etc/pve/` using
`file_set_contents`:

           print                big_writes+print     big_writes+syswrite
data size  written     amplif.  written     amplif.  written    amplif.
    1 KiB      48 KiB     48.0      45 KiB     45.0     41 KiB     41.0
    2 KiB      48 KiB     24.0      45 KiB     22.5     62 KiB     31.0
    4 KiB      82 KiB     20.5      80 KiB     20.0     73 KiB     18.3
    8 KiB     121 KiB     15.1      90 KiB     11.3     89 KiB     11.1
   16 KiB     217 KiB     13.6     146 KiB      9.1    113 KiB      7.1
   32 KiB     506 KiB     15.8     314 KiB      9.8    158 KiB      4.9
   64 KiB    1472 KiB     23.0     826 KiB     12.9    259 KiB      4.0
  128 KiB    5585 KiB     43.6    3765 KiB     29.4    452 KiB      3.5
  256 KiB   20424 KiB     79.8   10743 KiB     42.0   2351 KiB      9.2
  512 KiB   86715 KiB    169.4   43650 KiB     85.3   3204 KiB      6.3
 1024 KiB  369568 KiB    360.9  187496 KiB    183.1  15845 KiB     15.5

[1] https://lists.proxmox.com/pipermail/pve-devel/2024-September/065396.html

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/PVE/Tools.pm | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index bd305bd..0edf166 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -285,10 +285,17 @@ sub file_set_contents {
 	}
 	die "unable to open file '$tmpname' - $!\n" if !$fh;
 
-	binmode($fh, ":encoding(UTF-8)") if $force_utf8;
+	$data = encode("utf8", $data) if $force_utf8;
 
-	die "unable to write '$tmpname' - $!\n" unless print $fh $data;
-	die "closing file '$tmpname' failed - $!\n" unless close $fh;
+	my $offset = 0;
+	my $len = length($data);
+
+	while ($offset < $len) {
+	    $offset += syswrite($fh, $data, $len - $offset, $offset)
+		or die "unable to write '$tmpname' - $!\n";
+	}
+
+	close $fh or die "closing file '$tmpname' failed - $!\n";
     };
     my $err = $@;
 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print
  2024-09-24 14:33 [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print Filip Schauer
@ 2024-09-25  8:39 ` Dietmar Maurer
  2024-09-25 12:54   ` Filip Schauer
  2024-09-25 14:37 ` Dominik Csapak
  1 sibling, 1 reply; 5+ messages in thread
From: Dietmar Maurer @ 2024-09-25  8:39 UTC (permalink / raw)
  To: Proxmox VE development discussion, Filip Schauer

Please can you add the column showing write amplification using dd instead of file_set_contents, so that we can also see the minimal write amplf. from sqlite.

> The table below illustrates the drastic reduction in write
> amplification when writing files of different sizes to `/etc/pve/` using
> `file_set_contents`:
> 
>            print                big_writes+print     big_writes+syswrite
> data size  written     amplif.  written     amplif.  written    amplif.
>     1 KiB      48 KiB     48.0      45 KiB     45.0     41 KiB     41.0
>     2 KiB      48 KiB     24.0      45 KiB     22.5     62 KiB     31.0
>     4 KiB      82 KiB     20.5      80 KiB     20.0     73 KiB     18.3
>     8 KiB     121 KiB     15.1      90 KiB     11.3     89 KiB     11.1
>    16 KiB     217 KiB     13.6     146 KiB      9.1    113 KiB      7.1
>    32 KiB     506 KiB     15.8     314 KiB      9.8    158 KiB      4.9
>    64 KiB    1472 KiB     23.0     826 KiB     12.9    259 KiB      4.0
>   128 KiB    5585 KiB     43.6    3765 KiB     29.4    452 KiB      3.5
>   256 KiB   20424 KiB     79.8   10743 KiB     42.0   2351 KiB      9.2
>   512 KiB   86715 KiB    169.4   43650 KiB     85.3   3204 KiB      6.3
>  1024 KiB  369568 KiB    360.9  187496 KiB    183.1  15845 KiB     15.5
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print
  2024-09-25  8:39 ` Dietmar Maurer
@ 2024-09-25 12:54   ` Filip Schauer
  0 siblings, 0 replies; 5+ messages in thread
From: Filip Schauer @ 2024-09-25 12:54 UTC (permalink / raw)
  To: Dietmar Maurer, Proxmox VE development discussion

I am appending the following to the commit message:

Since `file_set_contents` also performs a `rename` after writing, the
following table shows the results when the file is written without
renaming it afterwards:

            print                big_writes+print big_writes+syswrite
file size  written     amplif.  written     amplif.  written amplif.
     1 KiB      29 KiB     29.0      29 KiB     29.0     25 KiB      25.0
     2 KiB      29 KiB     14.5      30 KiB     15.0     25 KiB      12.5
     4 KiB      37 KiB      9.3      44 KiB     11.0     41 KiB      10.3
     8 KiB      61 KiB      7.6      45 KiB      5.6     45 KiB       5.6
    16 KiB     143 KiB      8.9      86 KiB      5.4     57 KiB       3.6
    32 KiB     396 KiB     12.4     225 KiB      7.0     69 KiB       2.2
    64 KiB    1281 KiB     20.0     673 KiB     10.5    105 KiB       1.6
   128 KiB    4789 KiB     37.4    3478 KiB     27.2    169 KiB       1.3
   256 KiB   18868 KiB     73.7    9976 KiB     39.0    572 KiB       2.2
   512 KiB   79304 KiB    154.9   42714 KiB     83.4   2150 KiB       4.2
  1024 KiB  347929 KiB    339.8  182483 KiB    178.2  11133 KiB      10.9

On 25/09/2024 10:39, Dietmar Maurer wrote:
> Please can you add the column showing write amplification using dd instead of file_set_contents, so that we can also see the minimal write amplf. from sqlite.
>
>> The table below illustrates the drastic reduction in write
>> amplification when writing files of different sizes to `/etc/pve/` using
>> `file_set_contents`:
>>
>>             print                big_writes+print     big_writes+syswrite
>> data size  written     amplif.  written     amplif.  written    amplif.
>>      1 KiB      48 KiB     48.0      45 KiB     45.0     41 KiB     41.0
>>      2 KiB      48 KiB     24.0      45 KiB     22.5     62 KiB     31.0
>>      4 KiB      82 KiB     20.5      80 KiB     20.0     73 KiB     18.3
>>      8 KiB     121 KiB     15.1      90 KiB     11.3     89 KiB     11.1
>>     16 KiB     217 KiB     13.6     146 KiB      9.1    113 KiB      7.1
>>     32 KiB     506 KiB     15.8     314 KiB      9.8    158 KiB      4.9
>>     64 KiB    1472 KiB     23.0     826 KiB     12.9    259 KiB      4.0
>>    128 KiB    5585 KiB     43.6    3765 KiB     29.4    452 KiB      3.5
>>    256 KiB   20424 KiB     79.8   10743 KiB     42.0   2351 KiB      9.2
>>    512 KiB   86715 KiB    169.4   43650 KiB     85.3   3204 KiB      6.3
>>   1024 KiB  369568 KiB    360.9  187496 KiB    183.1  15845 KiB     15.5
>>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print
  2024-09-24 14:33 [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print Filip Schauer
  2024-09-25  8:39 ` Dietmar Maurer
@ 2024-09-25 14:37 ` Dominik Csapak
  2024-09-30 11:38   ` Filip Schauer
  1 sibling, 1 reply; 5+ messages in thread
From: Dominik Csapak @ 2024-09-25 14:37 UTC (permalink / raw)
  To: Proxmox VE development discussion, Filip Schauer


hi,

as we already talked off list, this may be an issue when we do have a string
that is marked as utf8 (e.g. api parameters that contain codepoints > 127)

if such a parameter reach the syswrite it will fail, so we should probably
use is_utf8 (or something better?) to detect if a string is utf8 and encode
it before passing it to syswrite.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print
  2024-09-25 14:37 ` Dominik Csapak
@ 2024-09-30 11:38   ` Filip Schauer
  0 siblings, 0 replies; 5+ messages in thread
From: Filip Schauer @ 2024-09-30 11:38 UTC (permalink / raw)
  To: Dominik Csapak, Proxmox VE development discussion

Superseded by:
https://lists.proxmox.com/pipermail/pve-devel/2024-September/065476.html

On 25/09/2024 16:37, Dominik Csapak wrote:
>
> hi,
>
> as we already talked off list, this may be an issue when we do have a 
> string
> that is marked as utf8 (e.g. api parameters that contain codepoints > 
> 127)
>
> if such a parameter reach the syswrite it will fail, so we should 
> probably
> use is_utf8 (or something better?) to detect if a string is utf8 and 
> encode
> it before passing it to syswrite.
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-09-30 11:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-09-24 14:33 [pve-devel] [PATCH common] tools: file_set_contents: use syswrite instead of print Filip Schauer
2024-09-25  8:39 ` Dietmar Maurer
2024-09-25 12:54   ` Filip Schauer
2024-09-25 14:37 ` Dominik Csapak
2024-09-30 11:38   ` Filip Schauer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal