Unlitter /var/lock from all-debug.sh and history_event-zfs-list-cacher.sh.in. Allow history_event-zfs-list-cacher.sh.in per pool #12042
Conversation
nabijaczleweli
force-pushed
the
lockpsko
branch
5 times, most recently
from
5517014 to
ee55b01
Compare
nabijaczleweli
force-pushed
the
lockpsko
branch
2 times, most recently
from
36b161b to
ccf35c6
Compare
nabijaczleweli
changed the title
nabijaczleweli
force-pushed
the
lockpsko
branch
2 times, most recently
from
e724424 to
5d952af
Compare
nabijaczleweli
force-pushed
the
lockpsko
branch
2 times, most recently
from
278d92c to
f421eef
Compare
nabijaczleweli
changed the title
nabijaczleweli
changed the title
Before, make shellcheck checked
scripts/{commitcheck,make_gitrev,man-dates,paxcheck,zfs-helpers,zfs,
zfs-tests,zimport,zloop}.sh
cmd/zed/zed.d/{{all-debug,all-syslog,data-notify,generic-notify,
resilver_finish-start-scrub,scrub_finish-notify,
statechange-led,statechange-notify,trim_finish-notify,
zed-functions}.sh,history_event-zfs-list-cacher.sh.in}
cmd/zpool/zpool.d/{dm-deps,iostat,lsblk,media,ses,smart,upath}
now it also checks
contrib/dracut/{02zfsexpandknowledge/module-setup,
90zfs/{export-zfs,parse-zfs,zfs-needshutdown,
zfs-load-key,zfs-lib,module-setup,
mount-zfs,zfs-generator}}.sh.in
cmd/zed/zed.d/{pool_import-led,vdev_attach-led,
resilver_finish-notify,vdev_clear-led}.sh
contrib/initramfs/{zfsunlock,hooks/zfs.in,scripts/local-top/zfs}
tests/zfs-tests/tests/perf/scripts/prefetch_io.sh
scripts/common.sh.in
contrib/bpftrace/zfs-trace.sh
autogen.sh
Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #12042
|
My mistake, I added the wrong PR number to the recently merged comments and accidentally closed this. Re-opening. |
|
Rebased |
|
Unfortunately, using "Lock files should be stored within the /var/lock directory structure." |
In this particular case, I would get rid of the middle "lock zed.debug.log" commit, and squash the other two. That way the dependency wont be missed when other people backport this PR to the release branch. |
|
In both cases locking purely on the output file makes more sense. I'd also say that the FHS directs our use of As for squashing all of these, I'm moderately apprehensive about it, since, well, they all do logically disparate things, and especially the cacher patch is non-trivial. |
Ah, fair point
Yea, after looking at your code in more detail, I see what you're getting at. Why don't you leave the three commits as-is, and we'll just try to keep it in the back of our minds that there's a dependency when we backport. |
| @@ -126,7 +126,7 @@ zed_lock() | |||
|
|
|||
| # Obtain a lock on the file bound to the given file descriptor. | |||
| # | |||
| eval "exec ${fd}> '${lockfile}'" | |||
There was a problem hiding this comment.
Just so I understand correctly, you use the append (>>) instead of the truncate (>) is to keep us from needlessly recreating the lockfile if it already exists? Is that right?
There was a problem hiding this comment.
In so many words, yeah. It allows us to lock on any file (that we have write permissions for) instead of only dedicated lock files, since the locking process itself no longer alters the file in any way.
There was a problem hiding this comment.
Ok, could you add that to the commit message, or add it as a comment to the code, just so future generations know what's happening:
zed-functions.sh: zed_lock(): don't truncate lock
Use append (>>) instead of truncate (>) to let us lock on any file
(that we have write permissions for) instead of only dedicated lock
files, since the locking process itself no longer alters the file in
any way.
There was a problem hiding this comment.
Sure, updated
By appending instead of truncating, we can lock on any file (with write permissions) instead of only dedicated lock files, since the locking process itself no longer alters the file in any way Signed-off-by: Ahelenia Ziemiańska <[email protected]>
By locking the log file itself, we can omit arduous rebinding and
explicit umask setting, but, perhaps more importantly, avoid permanently
littering /var/lock/ with zed.debug.log.lock we will never delete
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
This:
(a) improves the error log message,
(b) locks per pool instead of globally,
(c) locks the actual output file instead of /var/lock/zfs-list,
which would otherwise linger there forever (well, still will,
but you can remove it and it won't come back), and
(d) preserves attributes of the output file
instead of reverting them to 0:0 644
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12042
Before, make shellcheck checked
scripts/{commitcheck,make_gitrev,man-dates,paxcheck,zfs-helpers,zfs,
zfs-tests,zimport,zloop}.sh
cmd/zed/zed.d/{{all-debug,all-syslog,data-notify,generic-notify,
resilver_finish-start-scrub,scrub_finish-notify,
statechange-led,statechange-notify,trim_finish-notify,
zed-functions}.sh,history_event-zfs-list-cacher.sh.in}
cmd/zpool/zpool.d/{dm-deps,iostat,lsblk,media,ses,smart,upath}
now it also checks
contrib/dracut/{02zfsexpandknowledge/module-setup,
90zfs/{export-zfs,parse-zfs,zfs-needshutdown,
zfs-load-key,zfs-lib,module-setup,
mount-zfs,zfs-generator}}.sh.in
cmd/zed/zed.d/{pool_import-led,vdev_attach-led,
resilver_finish-notify,vdev_clear-led}.sh
contrib/initramfs/{zfsunlock,hooks/zfs.in,scripts/local-top/zfs}
tests/zfs-tests/tests/perf/scripts/prefetch_io.sh
scripts/common.sh.in
contrib/bpftrace/zfs-trace.sh
autogen.sh
Reviewed-by: John Kennedy <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12042
Reviewed-by: John Kennedy <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12042
behlendorf
added
Status: Accepted
By locking the log file itself, we can omit arduous rebinding and
explicit umask setting, but, perhaps more importantly, avoid permanently
littering /var/lock/ with zed.debug.log.lock we will never delete
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12042
This:
(a) improves the error log message,
(b) locks per pool instead of globally,
(c) locks the actual output file instead of /var/lock/zfs-list,
which would otherwise linger there forever (well, still will,
but you can remove it and it won't come back), and
(d) preserves attributes of the output file
instead of reverting them to 0:0 644
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #12042
By appending instead of truncating, we can lock on any file (with write permissions) instead of only dedicated lock files, since the locking process itself no longer alters the file in any way Reviewed-by: Tony Hutter <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#12042
By locking the log file itself, we can omit arduous rebinding and
explicit umask setting, but, perhaps more importantly, avoid permanently
littering /var/lock/ with zed.debug.log.lock we will never delete
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12042
This:
(a) improves the error log message,
(b) locks per pool instead of globally,
(c) locks the actual output file instead of /var/lock/zfs-list,
which would otherwise linger there forever (well, still will,
but you can remove it and it won't come back), and
(d) preserves attributes of the output file
instead of reverting them to 0:0 644
It is imperative that the previous commit
("zed-functions.sh: zed_lock(): don't truncate lock")
be included in any series that contains this one
Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#12042
Motivation and Context
Pessimistic locks and needless litter in
/var/lock.Description
See commit messages.
How Has This Been Tested?
Ran all-debug.sh – it's okay. Haven't run history_event-zfs-list-cacher.sh.in enough (it should be fine, but what do you know; will dedraft when I've done so).Ran it, I guess?
Also: is there some better way to mark the latter two patches as critically dependent on the former than what I've done to the messages?
Types of changes
Checklist:
Signed-off-by.