[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

Wayne Davison  changed:

   What|Removed |Added

 Resolution|--- |FIXED
 Status|ASSIGNED|RESOLVED
Version|3.1.3   |3.2.0

--- Comment #9 from Wayne Davison  ---
Yes, an absolute-path partial dir is not removed by rsync.  I'll add a mention
of that to the manpage.

I'm committing the simple version of the patch.  Thanks for checking it out!

As to the question about which basis file is better to use when a partial file
exists, that is indeed a hard one to answer since it depends a lot on how much
of a transfer completed and how much matching data exists in each file.  So far
I've left it up to the user to remove a partial file that would make their
transfer worse off, but it might be possible to come up with a simple heuristic
when creating the partial file to guess if we think we should save it or not --
something based on the partial file size vs final file size and also how much
of its data was copied literally vs copied from the current basis file.  That
seems like it would be pretty useful to me, especially if it erred on the side
of only removing the partial file if it was very clear it was not going to be
useful.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #8 from Ben RUBSON  ---
Wayne, I then just tested your patch, and it works as expected.

One thing perhaps, just to be sure.
I used an absolute partial-dir path during my tests.
It is not deleted after the partial file has been completed and moved to its
destination place.
Is it intended ?
Perhaps you assume that this absolute partial-dir could contain other partial
files, or even non rsync-related files ?
I'm OK with this behavior, IMO more secure :)

Your patch then re-creates the behavior I was looking for / proposed through my
patch.
Mine was intended to make this feature "compatible" with older clients, which
made it a little bit more complex...
Yours is more simple, which is better, perfect then, many thanks for having
looked into this !

To go further, some reflexions.
Let's imagine we use --link-dest and --partial-dir, with this current patch.
File2, size 100MB, is sending, while comparing to a nice basis File1 (let's
imagine almost perfect matching) found in one of the link-dest directories.
Transfer aborts.
Partial File2, 10MB, is moved into partial-dir.
Transfer restarts, and File2 is sent again, this time comparing to the partial
File2 (yes this is what we want here, perfect).
But then, perhaps we could have been faster comparing again to File1 instead ?
:)
Hard choice, and there's no way to know...
I tried to address this in the following patch :
https://bugzilla.samba.org/show_bug.cgi?id=13083
In this situation, the patch takes both files as basis files, which then can
speed-up the transfer.
Of course it depends on files' size, checksum speed, remaining data to send
etc...
The main avantage is in the situation above, where there's a lot of remaining
data to send.

Thank you again !

Ben

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #7 from Ben RUBSON  ---
Wayne, thank you very much for your detailed answer and for your patch
proposal.
Let me go through this and come back to you shortly.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #6 from Wayne Davison  ---
Created attachment 15941
  --> https://bugzilla.samba.org/attachment.cgi?id=15941=edit
A simpler patch that just uses in-place updates of partial-dir files when
possible

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

Wayne Davison  changed:

   What|Removed |Added

 Status|NEW |ASSIGNED

--- Comment #5 from Wayne Davison  ---
I dislike confusing what --inplace means. While an expert will know that it
changes meaning in different circumstances, I think it best that it always
means that the final destination file should be updated in-place, and thus
rsync should tell the user when it can't be used in combination with other
options.

As for the changes in this patch, I see 2 aspects of its interaction with the
partial-dir.  First, what happens when transferring a file that has a partial
file that already exists? And second, is the partial-dir used to store all the
temp files that are created for a transfer rather than the normal tmp file
being created and the partial-dir only getting created if the transfer is
interrupted?

As for the first part, I'm thinking that it would be fine if we change how a
partial-dir file is handled to always update it in-place, especially since
there is a good chance that the transferred file's early data is OK. The hard
part of that is that the sender needs to know which files are in-place
transfers so that it can restrict the checksum matching algorithm, so we must
know that the sender is new enough to respond to a FNAMECMP_PARTIAL_DIR value
as an indicator that the file is going to be handled in-place.  This could be
done via a compat flag character sent via -e or by forcing it to be an option
that an older sender would reject as unknown.

As for the second part, I'm not sure how much of a need there is for changing
the default behavior of the tmp file creation, and would probably not include
that change.  Also, the patch has an error in it where files in a subdir of the
transfer do not get created if the partial-dir is a relative path instead of an
absolute path (this is due to its use of "dummy" in the call that creates the
dir).

One interesting aspect of the patch is that it allows the --append option to be
used on partial files.  This could be useful if you're 100% certain that the
huge partial file is still a valid prefix of the source file.  However,
specifying --append with a --partial-dir would also affect how non-partial-dir
files get transferred, and is thus not a good way to affect just the continuing
of a partial-dir file's contents.  If we want appending of partial file data,
it would be better to add a --partial-append option and have both sides only
apply append behavior to the FNAMECMP_PARTIAL_DIR transfers.

Finally, since this patch sets the inplace flag for all files, it also has the
side effect that even non-partial files get the limited checksum matching
rules, which can make some transfers of shifted data much less efficient.

I'm looking at a simpler change which I will attach as a patch. I would
appreciate some feedback if the simpler behavior is enough for what you wanted
from this change.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #4 from Ben RUBSON  ---
Hi Wayne,
Let's review & merge this new feature ?
Thank you very much !

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #3 from Ben RUBSON  ---
Thank you for your feedback Ferran !

I'm not sure about why this patch hasn't been landed yet :)

Regarding benefits of using this option in conjunction with --link-dest.
Well, certainly same benefit as without :)
But imagine you are in a daily backup scheme, using --link-dest.
Everyday you have a new timestamped backup directory.
Day n, backup window is over, but you did not finish to transfer a huge file.
Thanks to this patch, day n+1 will resume the huge file flawlessly, inplace.

Hope to see this merged :)

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

Ferran  changed:

   What|Removed |Added

 CC||pallares.fer...@gmail.com

--- Comment #2 from Ferran  ---
I've come across the very same idea exposed in this request and was about to
code a patch for it. A quick search in the bug tracker showed up this patch,
which looks very sensible to me, and I would like to show my support for it.

Aside from the clear space-saving benefits, being able to transfer files
directly into the partial directory will speed-up resuming an interrupted
transfer by saving an unnecessary copy (from the partial dir to the
.hidden.file.hgsxjk). Saving this copy can make the difference when the files
are large.

However, I don't get the benefits of using this option in conjunction with
--link-dest, could you give us some more detail?

I've just tested the patch myself in the development version (3.1.4dev) and the
feature works as expected. The patch looks quite simple and self-contained, and
doesn't seem to interfere much with other features. Is there any reason (aside
from time limitations) why this patch hasn't been landed yet?

Thanks!,
Ferran

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html

[Bug 13071] [PATCH] Allow --partial-dir with --inplace

[just subscribed for rsync-qa from bugzilla via rsync]

https://bugzilla.samba.org/show_bug.cgi?id=13071

--- Comment #1 from Ben RUBSON  ---
Tested and still correctly works with rsync 3.1.3.
Thx !

-- 
You are receiving this mail because:
You are the QA Contact for the bug.

-- 
Please use reply-all for most replies to avoid omitting the mailing list.
To unsubscribe or change options: https://lists.samba.org/mailman/listinfo/rsync
Before posting, read: http://www.catb.org/~esr/faqs/smart-questions.html