Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Dietmar Maurer]

> > +        flush(self.output.as_mut()).await?;
> 
> According to the patch comment this hasn't broken anywhere at the time,
> but was there any test-code that did need this?
> 
> I'd like to make this at least conditional on the writer being
> `EncoderOutput::Owned` to not cause additional flushes for every single
> level of directory nesting.

Oh, I was not aware that this calls flush for every directory. I guess
nobody really wants that.

> That said, I'm not even convinced an `Owned` writer would really need
> this? You don't need to explicitly call `flush()` on a `std::fs::File`
> or even a `std::io::BufWriter` explicitly (`BufWriter` explicitly
> flushes in its `Drop` handler), unless you *explicitly* want to handle
> its error, but then you should keep ownership of the writer you pass to
> the encoder anyway and flush manually, not leave that up to the pxar
> code.

I am ok with reverting this patch.

Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Wolfgang Bumiller]

> On 03/30/2021 9:10 AM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > After an off-list talk with Dominik we concluded that keeping it for `Owned`
> > writers is the safer approach for the simple reason that in *async* code (eg.
> > tokio's async BufWriter equivalent) you *do* need to flush buffered writers.
> > 
> > This is probably because there's no `AsyncDrop` and there's no guarantee that
> > the future's `Drop` handler is called in a place where it is safe to call
> > tokio's `block_in_place` (after all it panics when it is outside a tokio RT
> > thread, *including* being inside a `runtime.block_on()` called from outside
> > a tokio RT thread).
> > 
> > So yeah, let's not revert this, but limit it to `EncoderOutput::Owned`.
> 
> But Flushing at every sub-dir makes no real sense. You only need
> to flush once when you are finished writing the pxar (for the top level dir)?

Yes, this is implicitly the case then because nested directories borrow the writer
and therefor always use `EncoderOutput::Borrowed`. Only the top level has the
`Owned` entry. (If this ever changes we can just track this with a boolean or something)

Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Dietmar Maurer]

> After an off-list talk with Dominik we concluded that keeping it for `Owned`
> writers is the safer approach for the simple reason that in *async* code (eg.
> tokio's async BufWriter equivalent) you *do* need to flush buffered writers.
> 
> This is probably because there's no `AsyncDrop` and there's no guarantee that
> the future's `Drop` handler is called in a place where it is safe to call
> tokio's `block_in_place` (after all it panics when it is outside a tokio RT
> thread, *including* being inside a `runtime.block_on()` called from outside
> a tokio RT thread).
> 
> So yeah, let's not revert this, but limit it to `EncoderOutput::Owned`.

But Flushing at every sub-dir makes no real sense. You only need
to flush once when you are finished writing the pxar (for the top level dir)?

Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Wolfgang Bumiller]

> On 03/29/2021 6:25 PM Dietmar Maurer <dietmar@proxmox.com> wrote:
> 
>  
> > > +        flush(self.output.as_mut()).await?;
> > 
> > According to the patch comment this hasn't broken anywhere at the time,
> > but was there any test-code that did need this?
> > 
> > I'd like to make this at least conditional on the writer being
> > `EncoderOutput::Owned` to not cause additional flushes for every single
> > level of directory nesting.
> 
> Oh, I was not aware that this calls flush for every directory. I guess
> nobody really wants that.
> 
> > That said, I'm not even convinced an `Owned` writer would really need
> > this? You don't need to explicitly call `flush()` on a `std::fs::File`
> > or even a `std::io::BufWriter` explicitly (`BufWriter` explicitly
> > flushes in its `Drop` handler), unless you *explicitly* want to handle
> > its error, but then you should keep ownership of the writer you pass to
> > the encoder anyway and flush manually, not leave that up to the pxar
> > code.
> 
> I am ok with reverting this patch.

After an off-list talk with Dominik we concluded that keeping it for `Owned`
writers is the safer approach for the simple reason that in *async* code (eg.
tokio's async BufWriter equivalent) you *do* need to flush buffered writers.

This is probably because there's no `AsyncDrop` and there's no guarantee that
the future's `Drop` handler is called in a place where it is safe to call
tokio's `block_in_place` (after all it panics when it is outside a tokio RT
thread, *including* being inside a `runtime.block_on()` called from outside
a tokio RT thread).

So yeah, let's not revert this, but limit it to `EncoderOutput::Owned`.

Re: [pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Wolfgang Bumiller]

On Wed, Mar 24, 2021 at 11:56:38AM +0100, Dominik Csapak wrote:
> some writers may need to be flushed to write out all data
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> for now, it is not needed because in all code paths we use it
> the writers do not need to be flushed, but there is no guarantee
> it will stay that way
> 
>  src/encoder/mod.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs
> index 1004efa..c114657 100644
> --- a/src/encoder/mod.rs
> +++ b/src/encoder/mod.rs
> @@ -83,6 +83,13 @@ async fn seq_write<T: SeqWrite + ?Sized>(
>      Ok(put)
>  }
>  
> +/// awaitable version of 'poll_flush'.
> +async fn flush<T: SeqWrite + ?Sized>(
> +    output: &mut T,
> +) -> io::Result<()> {
> +    poll_fn(|cx| unsafe { Pin::new_unchecked(&mut *output).poll_flush(cx) }).await
> +}
> +
>  /// Write the entire contents of a buffer, handling short writes.
>  async fn seq_write_all<T: SeqWrite + ?Sized>(
>      output: &mut T,
> @@ -715,6 +722,8 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
>          )
>          .await?;
>  
> +        flush(self.output.as_mut()).await?;

According to the patch comment this hasn't broken anywhere at the time,
but was there any test-code that did need this?

I'd like to make this at least conditional on the writer being
`EncoderOutput::Owned` to not cause additional flushes for every single
level of directory nesting.
That said, I'm not even convinced an `Owned` writer would really need
this? You don't need to explicitly call `flush()` on a `std::fs::File`
or even a `std::io::BufWriter` explicitly (`BufWriter` explicitly
flushes in its `Drop` handler), unless you *explicitly* want to handle
its error, but then you should keep ownership of the writer you pass to
the encoder anyway and flush manually, not leave that up to the pxar
code.

[pbs-devel] [PATCH pxar] encoder: flush after writing last entry

[Dominik Csapak]

some writers may need to be flushed to write out all data

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
for now, it is not needed because in all code paths we use it
the writers do not need to be flushed, but there is no guarantee
it will stay that way

 src/encoder/mod.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/encoder/mod.rs b/src/encoder/mod.rs
index 1004efa..c114657 100644
--- a/src/encoder/mod.rs
+++ b/src/encoder/mod.rs
@@ -83,6 +83,13 @@ async fn seq_write<T: SeqWrite + ?Sized>(
     Ok(put)
 }
 
+/// awaitable version of 'poll_flush'.
+async fn flush<T: SeqWrite + ?Sized>(
+    output: &mut T,
+) -> io::Result<()> {
+    poll_fn(|cx| unsafe { Pin::new_unchecked(&mut *output).poll_flush(cx) }).await
+}
+
 /// Write the entire contents of a buffer, handling short writes.
 async fn seq_write_all<T: SeqWrite + ?Sized>(
     output: &mut T,
@@ -715,6 +722,8 @@ impl<'a, T: SeqWrite + 'a> EncoderImpl<'a, T> {
         )
         .await?;
 
+        flush(self.output.as_mut()).await?;
+
         // done up here because of the self-borrow and to propagate
         let end_offset = self.position();
 
-- 
2.20.1