Re: [pbs-devel] applied-series: [PATCH proxmox-backup v9 0/4] refactor datastore locking to use tmpfs

["Shannon Sterz" <s.sterz@proxmox.com>]

On Fri Mar 28, 2025 at 8:51 AM CET, Wolfgang Bumiller wrote:
> On Thu, Mar 27, 2025 at 11:02:35AM +0100, Shannon Sterz wrote:
>> On Wed Mar 26, 2025 at 4:36 PM CET, Thomas Lamprecht wrote:
>> > Am 26.03.25 um 12:44 schrieb Shannon Sterz:> The goal of this series is to make it safer to remove backup groups &
>> >> snapshots by separating the corresponding directories from their lock
>> >> files. By moving the lock files to the tmpfs-backed '/run' directory,
>> >> we also make sure that the lock files get cleaned up when the system
>> >> reboots.
>> >>
>> >> This series refactors the locking mechanism inside the `DataStore`,
>> >> `BackupDir` and `BackupGroup` traits. In a first step locking methods
>> >> are added and the existing code is refactored to use them. Commit two
>> >> derives a lock file name under '/run' for each group/snapshot. It also
>> >> adds double stat'ing. To avoid issues when upgrading, the file
>> >> `/run/proxmox-backup/old-locking` is created through a post-install
>> >> hook which is used to determine whether the system has been rebooted
>> >> and we can safely use the new locking mechanism.
>> >>
>> >> The third commit refactors locking for manifests and brings it in-line
>> >> with the group/snapshot locks. Finally, the last commit fixes a race
>> >> condition when changing the owner of a datastore.
>> >
>> >> Shannon Sterz (4):
>> >>   datastore/api/backup: prepare for fix of #3935 by adding lock helpers
>> >>   fix #3935: datastore/api/backup: move datastore locking to '/run'
>> >>   fix #3935: datastore: move manifest locking to new locking method
>> >>   fix: api: avoid race condition in set_backup_owner
>> >>
>> >
>> > applied series, added Wolfgang's Acked-by – feel encouraged to pick such
>> > things up already for a new revision, especially an A-b is less problematic
>> > with minor changes as long as the overall design stays the same – and also
>> > fixed a minor compile warning about an unused import that stemmed from
>> > intra-revision changes, thanks!
>>
>> thanks a bunch! should i always add all trailers mentioned on list?
>> wasn't sure about the `Acked-by` tbh ^^'
>
> At the very least any commits with no changes can accumulate all the
> trailers.
>
> For any changed commits you'll need to gauge whether or not another look
> is warranted.
> Eg. obviously typo or small rustfmt fixes are fine.
> In the previous series I also specifically stated that my Ack includes
> the proposed changes as I figured they'd be small enough to not require
> another complete review, so as a review I would've just done a git-diff
> between the 2 completely-applied versions rather than looked at the
> individual patches first. (I don't necessarily do this between versions,
> depending on how the changes are described in the emails, since with
> regular read-throughs I'll usually pick up more things which I might
> have missed in earlier versions.)

makes sense, thanks! i'll try to keep that in mind an do better on
adding trailers in the future!


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