Message ID | 73rweeabpoypzqwyxa7hld7tnkskkaotuo3jjfxnpgn6gg47ly@admkywnz4fsp (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | [GIT,PULL] bcachefs changes for 6.11, v2 | expand |
On Thu, Jul 18, 2024 at 06:36:50PM GMT, Kent Overstreet wrote: > Ok, version 2: > > The following changes since commit 0c3836482481200ead7b416ca80c68a29cfdaabd: > > Linux 6.10 (2024-07-14 15:43:32 -0700) > > are available in the Git repository at: > > https://evilpiepirate.org/git/bcachefs.git tags/bcachefs-2024-07-18 Sorry, I'm fat fingering everything today, that tag doesn't include the lockdep comments patch. Pull from https://evilpiepirate.org/git/bcachefs.git tags/bcachefs-2024-07-18.2
The pull request you sent on Thu, 18 Jul 2024 18:36:50 -0400:
> https://evilpiepirate.org/git/bcachefs.git tags/bcachefs-2024-07-18
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/a97b43fac5b9b3ffca71b8a917a249789902fce9
Thank you!
The pull request you sent on Thu, 18 Jul 2024 18:39:20 -0400:
> https://evilpiepirate.org/git/bcachefs.git tags/bcachefs-2024-07-18.2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/720261cfc7329406a50c2a8536e0039b9dd9a4e5
Thank you!
On Thu, Jul 19, 2024 at 06:36:50PM -0400, Kent Overstreet wrote:
> bcachefs: Unlock trans when waiting for user input in fsck
Hello Kent Overstreet,
ommit 889fb3dc5d6f ("bcachefs: Unlock trans when waiting for user
input in fsck") from May 29, 2024 (linux-next), leads to the
following (UNPUBLISHED) Smatch static checker warning:
fs/bcachefs/error.c:129 bch2_fsck_ask_yn() error: double unlocked 'trans' (orig line 113)
fs/bcachefs/error.c
102 static enum ask_yn bch2_fsck_ask_yn(struct bch_fs *c, struct btree_trans *trans)
103 {
104 struct stdio_redirect *stdio = c->stdio;
105
106 if (c->stdio_filter && c->stdio_filter != current)
107 stdio = NULL;
108
109 if (!stdio)
110 return YN_NO;
111
112 if (trans)
113 bch2_trans_unlock(trans);
^^^^^^^^^^^^^^^^^^^^^^^^^
Unlock
114
115 unsigned long unlock_long_at = trans ? jiffies + HZ * 2 : 0;
116 darray_char line = {};
117 int ret;
118
119 do {
120 unsigned long t;
121 bch2_print(c, " (y,n, or Y,N for all errors of this type) ");
122 rewait:
123 t = unlock_long_at
124 ? max_t(long, unlock_long_at - jiffies, 0)
125 : MAX_SCHEDULE_TIMEOUT;
126
127 int r = bch2_stdio_redirect_readline_timeout(stdio, &line, t);
128 if (r == -ETIME) {
129 bch2_trans_unlock_long(trans);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Double unlock
130 unlock_long_at = 0;
131 goto rewait;
132 }
133
134 if (r < 0) {
135 ret = YN_NO;
136 break;
137 }
138
139 darray_last(line) = '\0';
140 } while ((ret = parse_yn_response(line.data)) < 0);
141
142 darray_exit(&line);
143 return ret;
144 }
regards,
dan carpenter
On Tue, Aug 27, 2024 at 01:53:55PM GMT, Dan Carpenter wrote: > On Thu, Jul 19, 2024 at 06:36:50PM -0400, Kent Overstreet wrote: > > bcachefs: Unlock trans when waiting for user input in fsck > > Hello Kent Overstreet, > > ommit 889fb3dc5d6f ("bcachefs: Unlock trans when waiting for user > input in fsck") from May 29, 2024 (linux-next), leads to the > following (UNPUBLISHED) Smatch static checker warning: > > fs/bcachefs/error.c:129 bch2_fsck_ask_yn() error: double unlocked 'trans' (orig line 113) > > fs/bcachefs/error.c > 102 static enum ask_yn bch2_fsck_ask_yn(struct bch_fs *c, struct btree_trans *trans) > 103 { > 104 struct stdio_redirect *stdio = c->stdio; > 105 > 106 if (c->stdio_filter && c->stdio_filter != current) > 107 stdio = NULL; > 108 > 109 if (!stdio) > 110 return YN_NO; > 111 > 112 if (trans) > 113 bch2_trans_unlock(trans); > ^^^^^^^^^^^^^^^^^^^^^^^^^ > Unlock > > 114 > 115 unsigned long unlock_long_at = trans ? jiffies + HZ * 2 : 0; > 116 darray_char line = {}; > 117 int ret; > 118 > 119 do { > 120 unsigned long t; > 121 bch2_print(c, " (y,n, or Y,N for all errors of this type) "); > 122 rewait: > 123 t = unlock_long_at > 124 ? max_t(long, unlock_long_at - jiffies, 0) > 125 : MAX_SCHEDULE_TIMEOUT; > 126 > 127 int r = bch2_stdio_redirect_readline_timeout(stdio, &line, t); > 128 if (r == -ETIME) { > 129 bch2_trans_unlock_long(trans); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Double unlock Those are different types of unlocks. The first unlock drops btree locks, but we still have pointers and lock sequence numbers to those nodes so that we can do a bch2_trans_relock() later, and continue the same transaction. But we still have an SRCU read lock held which prevents those nodes from being reclaimed, and we can't hold that for too long either. So if we're blocked here too long we have to also do an unlock_long(), which forces a transaction restart.
On Tue, Aug 27, 2024 at 12:23:22PM -0400, Kent Overstreet wrote: > On Tue, Aug 27, 2024 at 01:53:55PM GMT, Dan Carpenter wrote: > > On Thu, Jul 19, 2024 at 06:36:50PM -0400, Kent Overstreet wrote: > > > bcachefs: Unlock trans when waiting for user input in fsck > > > > Hello Kent Overstreet, > > > > ommit 889fb3dc5d6f ("bcachefs: Unlock trans when waiting for user > > input in fsck") from May 29, 2024 (linux-next), leads to the > > following (UNPUBLISHED) Smatch static checker warning: > > > > fs/bcachefs/error.c:129 bch2_fsck_ask_yn() error: double unlocked 'trans' (orig line 113) > > > > fs/bcachefs/error.c > > 102 static enum ask_yn bch2_fsck_ask_yn(struct bch_fs *c, struct btree_trans *trans) > > 103 { > > 104 struct stdio_redirect *stdio = c->stdio; > > 105 > > 106 if (c->stdio_filter && c->stdio_filter != current) > > 107 stdio = NULL; > > 108 > > 109 if (!stdio) > > 110 return YN_NO; > > 111 > > 112 if (trans) > > 113 bch2_trans_unlock(trans); > > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > Unlock > > > > 114 > > 115 unsigned long unlock_long_at = trans ? jiffies + HZ * 2 : 0; > > 116 darray_char line = {}; > > 117 int ret; > > 118 > > 119 do { > > 120 unsigned long t; > > 121 bch2_print(c, " (y,n, or Y,N for all errors of this type) "); > > 122 rewait: > > 123 t = unlock_long_at > > 124 ? max_t(long, unlock_long_at - jiffies, 0) > > 125 : MAX_SCHEDULE_TIMEOUT; > > 126 > > 127 int r = bch2_stdio_redirect_readline_timeout(stdio, &line, t); > > 128 if (r == -ETIME) { > > 129 bch2_trans_unlock_long(trans); > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > Double unlock > > Those are different types of unlocks. > What tripped the static checker up is that bch2_trans_unlock_long() calls bch2_trans_unlock(). fs/bcachefs/btree_locking.c 815 void bch2_trans_unlock_long(struct btree_trans *trans) 816 { 817 bch2_trans_unlock(trans); 818 bch2_trans_srcu_unlock(trans); 819 } But looking at it now, I guess if we call bch2_trans_unlock() twice the second unlock is a no-op. Thanks! regards, dan carpenter