* [PATCH v3] Updated locking documentation for transaction_t @ 2019-04-08 8:35 Alexander Lochmann 2019-06-20 20:45 ` Alexander Lochmann ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: Alexander Lochmann @ 2019-04-08 8:35 UTC (permalink / raw) To: tytso Cc: Alexander Lochmann, Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel We used LockDoc to derive locking rules for each member of struct transaction_t. Based on those results, we extended the existing documentation by more members of struct transaction_t, and updated the existing documentation. Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de> --- include/linux/jbd2.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 0f919d5fe84f..34b728e2b702 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { * The transaction keeps track of all of the buffers modified by a * running transaction, and all of the buffers committed but not yet * flushed to home for finished transactions. + * (Locking Documentation improved by LockDoc) */ /* @@ -652,12 +653,12 @@ struct transaction_s unsigned long t_start; /* - * When commit was requested + * When commit was requested [journal_t.j_state_lock] */ unsigned long t_requested; /* - * Checkpointing stats [j_checkpoint_sem] + * Checkpointing stats [journal_t.j_list_lock] */ struct transaction_chp_stats_s t_chp_stats; -- 2.20.1 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Updated locking documentation for transaction_t 2019-04-08 8:35 [PATCH v3] Updated locking documentation for transaction_t Alexander Lochmann @ 2019-06-20 20:45 ` Alexander Lochmann 2020-10-15 13:26 ` Alexander Lochmann 2020-10-15 13:56 ` [RFC] Fine-grained locking documentation for jbd2 data structures Alexander Lochmann 2 siblings, 0 replies; 13+ messages in thread From: Alexander Lochmann @ 2019-06-20 20:45 UTC (permalink / raw) To: tytso; +Cc: Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1807 bytes --] Hi Ted, Have you had the chance to review the most recent version of the patch? Does it look reasonable to you? Cheers, Alex On 08.04.19 10:35, Alexander Lochmann wrote: > We used LockDoc to derive locking rules for each member > of struct transaction_t. > Based on those results, we extended the existing documentation > by more members of struct transaction_t, and updated the existing > documentation. > > Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> > Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de> > --- > include/linux/jbd2.h | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h > index 0f919d5fe84f..34b728e2b702 100644 > --- a/include/linux/jbd2.h > +++ b/include/linux/jbd2.h > @@ -534,6 +534,7 @@ struct transaction_chp_stats_s { > * The transaction keeps track of all of the buffers modified by a > * running transaction, and all of the buffers committed but not yet > * flushed to home for finished transactions. > + * (Locking Documentation improved by LockDoc) > */ > > /* > @@ -652,12 +653,12 @@ struct transaction_s > unsigned long t_start; > > /* > - * When commit was requested > + * When commit was requested [journal_t.j_state_lock] > */ > unsigned long t_requested; > > /* > - * Checkpointing stats [j_checkpoint_sem] > + * Checkpointing stats [journal_t.j_list_lock] > */ > struct transaction_chp_stats_s t_chp_stats; > > -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] Updated locking documentation for transaction_t 2019-04-08 8:35 [PATCH v3] Updated locking documentation for transaction_t Alexander Lochmann 2019-06-20 20:45 ` Alexander Lochmann @ 2020-10-15 13:26 ` Alexander Lochmann 2020-12-03 14:04 ` Theodore Y. Ts'o 2020-10-15 13:56 ` [RFC] Fine-grained locking documentation for jbd2 data structures Alexander Lochmann 2 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2020-10-15 13:26 UTC (permalink / raw) To: tytso; +Cc: Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel [-- Attachment #1.1.1: Type: text/plain, Size: 147 bytes --] Hi folks, I've updated the lock documentation according to our finding for transaction_t. Does this patch look good to you? Cheers, Alex [-- Attachment #1.1.2: transaction_t-doc.patch --] [-- Type: text/x-patch, Size: 1446 bytes --] commit 13ac907c45c5da7d691f6e10972de5e56e0072c6 Author: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> Date: Thu Oct 15 15:24:52 2020 +0200 Updated locking documentation for transaction_t We used LockDoc to derive locking rules for each member of struct transaction_t. Based on those results, we extended the existing documentation by more members of struct transaction_t, and updated the existing documentation. Signed-off-by: Alexander Lochmann <alexander.lochmann@tu-dortmund.de> Signed-off-by: Horst Schirmeier <horst.schirmeier@tu-dortmund.de> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h index 08f904943ab2..a11c78e4af4e 100644 --- a/include/linux/jbd2.h +++ b/include/linux/jbd2.h @@ -532,6 +532,7 @@ struct transaction_chp_stats_s { * The transaction keeps track of all of the buffers modified by a * running transaction, and all of the buffers committed but not yet * flushed to home for finished transactions. + * (Locking Documentation improved by LockDoc) */ /* @@ -650,12 +651,12 @@ struct transaction_s unsigned long t_start; /* - * When commit was requested + * When commit was requested [journal_t.j_state_lock] */ unsigned long t_requested; /* - * Checkpointing stats [j_checkpoint_sem] + * Checkpointing stats [journal_t.j_list_lock] */ struct transaction_chp_stats_s t_chp_stats; [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Updated locking documentation for transaction_t 2020-10-15 13:26 ` Alexander Lochmann @ 2020-12-03 14:04 ` Theodore Y. Ts'o 2020-12-03 14:38 ` Alexander Lochmann 0 siblings, 1 reply; 13+ messages in thread From: Theodore Y. Ts'o @ 2020-12-03 14:04 UTC (permalink / raw) To: Alexander Lochmann; +Cc: Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote: > Hi folks, > > I've updated the lock documentation according to our finding for > transaction_t. > Does this patch look good to you? I updated the annotations to match with the local usage, e.g: * When commit was requested [journal_t.j_state_lock] became: * When commit was requested [j_state_lock] Otherwise, looks good. Thanks for the patch! - Ted ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Updated locking documentation for transaction_t 2020-12-03 14:04 ` Theodore Y. Ts'o @ 2020-12-03 14:38 ` Alexander Lochmann 2020-12-03 20:39 ` Theodore Y. Ts'o 0 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2020-12-03 14:38 UTC (permalink / raw) To: Theodore Y. Ts'o; +Cc: Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1009 bytes --] On 03.12.20 15:04, Theodore Y. Ts'o wrote: > On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote: >> Hi folks, >> >> I've updated the lock documentation according to our finding for >> transaction_t. >> Does this patch look good to you? > > I updated the annotations to match with the local usage, e.g: > > * When commit was requested [journal_t.j_state_lock] > > became: > > * When commit was requested [j_state_lock]What do you mean by local usage? The annotations of other members of transaction_t? Shouldn't the annotation look like this? [t_journal->j_state_lock] It would be more precise. > > Otherwise, looks good. Thanks for the patch! Thanks! - Alex > > - Ted > -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] Updated locking documentation for transaction_t 2020-12-03 14:38 ` Alexander Lochmann @ 2020-12-03 20:39 ` Theodore Y. Ts'o 0 siblings, 0 replies; 13+ messages in thread From: Theodore Y. Ts'o @ 2020-12-03 20:39 UTC (permalink / raw) To: Alexander Lochmann; +Cc: Horst Schirmeier, Jan Kara, linux-ext4, linux-kernel On Thu, Dec 03, 2020 at 03:38:40PM +0100, Alexander Lochmann wrote: > > > On 03.12.20 15:04, Theodore Y. Ts'o wrote: > > On Thu, Oct 15, 2020 at 03:26:28PM +0200, Alexander Lochmann wrote: > > > Hi folks, > > > > > > I've updated the lock documentation according to our finding for > > > transaction_t. > > > Does this patch look good to you? > > > > I updated the annotations to match with the local usage, e.g: > > > > * When commit was requested [journal_t.j_state_lock] > > > > became: > > > > * When commit was requested [j_state_lock]What do you mean by local usage? > The annotations of other members of transaction_t? Yes, I'd like the annotations of the other objects to be consistent, and just use j_state_lock, j_list_lock, etc., for the other annotations. > Shouldn't the annotation look like this? > [t_journal->j_state_lock] > It would be more precise. It's more precise, but it's also unnecessary in this case, since all of the elements of the journal have a j_ prefix, elements of a transaction_t have a t_ prefix, etc. There is also no other structure element which has a j_state_lock name *other* than in journal_t. Cheers, - Ted ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC] Fine-grained locking documentation for jbd2 data structures 2019-04-08 8:35 [PATCH v3] Updated locking documentation for transaction_t Alexander Lochmann 2019-06-20 20:45 ` Alexander Lochmann 2020-10-15 13:26 ` Alexander Lochmann @ 2020-10-15 13:56 ` Alexander Lochmann 2021-02-05 15:31 ` Alexander Lochmann 2 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2020-10-15 13:56 UTC (permalink / raw) To: tytso, Jan Kara; +Cc: Horst Schirmeier, linux-ext4 [-- Attachment #1.1: Type: text/plain, Size: 1709 bytes --] Hi folks, when comparing our generated locking documentation with the current documentation located in include/linux/jbd2.h, I found some inconsistencies. (Our approach: https://dl.acm.org/doi/10.1145/3302424.3303948) According to the official documentation, the following members should be read using a lock: journal_t - j_flags: j_state_lock - j_barrier_count: j_state_lock - j_running_transaction: j_state_lock - j_commit_sequence: j_state_lock - j_commit_request: j_state_lock transactiont_t - t_nr_buffers: j_list_lock - t_buffers: j_list_lock - t_reserved_list: j_list_lock - t_shadow_list: j_list_lock jbd2_inode - i_transaction: j_list_lock - i_next_transaction: j_list_lock - i_flags: j_list_lock - i_dirty_start: j_list_lock - i_dirty_start: j_list_lock However, our results say that no locks are needed at all for *reading* those members. From what I know, it is common wisdom that word-sized data types can be read without any lock in the Linux kernel. All of the above members have word size, i.e., int, long, or ptr. Is it therefore safe to split the locking documentation as follows? @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] The following members are not word-sizes. Our results also suggest that no locks are needed. Can the proposed change be applied to them as well? transaction_t - t_chp_stats: j_checkpoint_sem jbd2_inode: - i_list: j_list_lock Cheers, Alex -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2020-10-15 13:56 ` [RFC] Fine-grained locking documentation for jbd2 data structures Alexander Lochmann @ 2021-02-05 15:31 ` Alexander Lochmann 2021-02-08 15:27 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2021-02-05 15:31 UTC (permalink / raw) To: tytso, Jan Kara; +Cc: Horst Schirmeier, linux-ext4 Hi Ted, hi Jack, have you had the chance to review our results? Cheers, Alex On 15.10.20 15:56, Alexander Lochmann wrote: > Hi folks, > > when comparing our generated locking documentation with the current > documentation > located in include/linux/jbd2.h, I found some inconsistencies. (Our > approach: https://dl.acm.org/doi/10.1145/3302424.3303948) > According to the official documentation, the following members should be > read using a lock: > journal_t > - j_flags: j_state_lock > - j_barrier_count: j_state_lock > - j_running_transaction: j_state_lock > - j_commit_sequence: j_state_lock > - j_commit_request: j_state_lock > transactiont_t > - t_nr_buffers: j_list_lock > - t_buffers: j_list_lock > - t_reserved_list: j_list_lock > - t_shadow_list: j_list_lock > jbd2_inode > - i_transaction: j_list_lock > - i_next_transaction: j_list_lock > - i_flags: j_list_lock > - i_dirty_start: j_list_lock > - i_dirty_start: j_list_lock > > However, our results say that no locks are needed at all for *reading* > those members. > From what I know, it is common wisdom that word-sized data types can be > read without any lock in the Linux kernel. > All of the above members have word size, i.e., int, long, or ptr. > Is it therefore safe to split the locking documentation as follows? > @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] > > The following members are not word-sizes. Our results also suggest that > no locks are needed. > Can the proposed change be applied to them as well? > transaction_t > - t_chp_stats: j_checkpoint_sem > jbd2_inode: > - i_list: j_list_lock > > Cheers, > Alex > -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2021-02-05 15:31 ` Alexander Lochmann @ 2021-02-08 15:27 ` Jan Kara 2021-02-09 9:58 ` Alexander Lochmann 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2021-02-08 15:27 UTC (permalink / raw) To: Alexander Lochmann; +Cc: tytso, Jan Kara, Horst Schirmeier, linux-ext4 Hi Alexander! On Fri 05-02-21 16:31:54, Alexander Lochmann wrote: > have you had the chance to review our results? It fell through the cracks I guess. Thanks for pinging. Let me have a look. > On 15.10.20 15:56, Alexander Lochmann wrote: > > Hi folks, > > > > when comparing our generated locking documentation with the current > > documentation > > located in include/linux/jbd2.h, I found some inconsistencies. (Our > > approach: https://dl.acm.org/doi/10.1145/3302424.3303948) > > According to the official documentation, the following members should be > > read using a lock: > > journal_t > > - j_flags: j_state_lock > > - j_barrier_count: j_state_lock > > - j_running_transaction: j_state_lock > > - j_commit_sequence: j_state_lock > > - j_commit_request: j_state_lock > > transactiont_t > > - t_nr_buffers: j_list_lock > > - t_buffers: j_list_lock > > - t_reserved_list: j_list_lock > > - t_shadow_list: j_list_lock > > jbd2_inode > > - i_transaction: j_list_lock > > - i_next_transaction: j_list_lock > > - i_flags: j_list_lock > > - i_dirty_start: j_list_lock > > - i_dirty_start: j_list_lock > > > > However, our results say that no locks are needed at all for *reading* > > those members. > > From what I know, it is common wisdom that word-sized data types can be > > read without any lock in the Linux kernel. Yes, although in last year, people try to convert these unlocked reads to READ_ONCE() or similar as otherwise the compiler is apparently allowed to generate code which is not safe. But that's a different story. Also note that although reading that particular word may be safe without any other locks, the lock still may be needed to safely interpret the value in the context of other fetched values (e.g., due to consistency among multiple structure members). So sometimes requiring the lock is just the least problematic solution - there's always the tradeoff between the speed and simplicity. > > All of the above members have word size, i.e., int, long, or ptr. > > Is it therefore safe to split the locking documentation as follows? > > @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] I've checked the code and we usually use unlocked reads for quick, possibly racy checks and if they indicate we may need to do something then take the lock and do a reliable check. This is quite common pattern, not sure how to best document this. Maybe like [j_state_lock, no lock for quick racy checks]? > > The following members are not word-sizes. Our results also suggest that > > no locks are needed. > > Can the proposed change be applied to them as well? > > transaction_t > > - t_chp_stats: j_checkpoint_sem Where do we read t_chp_stats outside of a lock? j_list_lock seems to be used pretty consistently there. Except perhaps __jbd2_journal_remove_checkpoint() but there we know we are already the only ones touching the transaction and thus its statistics. > > jbd2_inode: > > - i_list: j_list_lock And here as well. I would not complicate the locking description unless we really have places that access these fields without locks... Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2021-02-08 15:27 ` Jan Kara @ 2021-02-09 9:58 ` Alexander Lochmann 2021-02-09 12:00 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2021-02-09 9:58 UTC (permalink / raw) To: Jan Kara; +Cc: tytso, Jan Kara, Horst Schirmeier, linux-ext4 [-- Attachment #1.1: Type: text/plain, Size: 4195 bytes --] On 08.02.21 16:27, Jan Kara wrote: > Hi Alexander! > > On Fri 05-02-21 16:31:54, Alexander Lochmann wrote: >> have you had the chance to review our results? > > It fell through the cracks I guess. Thanks for pinging. Let me have a look. > >> On 15.10.20 15:56, Alexander Lochmann wrote: >>> Hi folks, >>> >>> when comparing our generated locking documentation with the current >>> documentation >>> located in include/linux/jbd2.h, I found some inconsistencies. (Our >>> approach: https://dl.acm.org/doi/10.1145/3302424.3303948) >>> According to the official documentation, the following members should be >>> read using a lock: >>> journal_t >>> - j_flags: j_state_lock >>> - j_barrier_count: j_state_lock >>> - j_running_transaction: j_state_lock >>> - j_commit_sequence: j_state_lock >>> - j_commit_request: j_state_lock >>> transactiont_t >>> - t_nr_buffers: j_list_lock >>> - t_buffers: j_list_lock >>> - t_reserved_list: j_list_lock >>> - t_shadow_list: j_list_lock >>> jbd2_inode >>> - i_transaction: j_list_lock >>> - i_next_transaction: j_list_lock >>> - i_flags: j_list_lock >>> - i_dirty_start: j_list_lock >>> - i_dirty_start: j_list_lock >>> >>> However, our results say that no locks are needed at all for *reading* >>> those members. >>> From what I know, it is common wisdom that word-sized data types can be >>> read without any lock in the Linux kernel. > > Yes, although in last year, people try to convert these unlocked reads to > READ_ONCE() or similar as otherwise the compiler is apparently allowed to > generate code which is not safe. But that's a different story. Is this ongoing work? Using such a macro would a) make our work much easier as we can instrument them, and b) would tell less experienced developers that no locking is needed. Does the usage of READ_ONCE() imply that no lock is needed? Otherwise, one could introduce another macro for jbd2, such as #define READ_UNLOCKED() READ_ONCE(), which is more precise. Also note > that although reading that particular word may be safe without any other > locks, the lock still may be needed to safely interpret the value in the > context of other fetched values (e.g., due to consistency among multiple > structure members). Just a side quest: Do you have an example for such a situation? So sometimes requiring the lock is just the least > problematic solution - there's always the tradeoff between the speed and > simplicity. > >>> All of the above members have word size, i.e., int, long, or ptr. >>> Is it therefore safe to split the locking documentation as follows? >>> @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] > > I've checked the code and we usually use unlocked reads for quick, possibly > racy checks and if they indicate we may need to do something then take the > lock and do a reliable check. This is quite common pattern, not sure how to > best document this. Maybe like [j_state_lock, no lock for quick racy checks]? > Yeah, I'm fine with that. Does this rule apply for the other members of journal_t (and transaction_t?) listed above? >>> The following members are not word-sizes. Our results also suggest that >>> no locks are needed. >>> Can the proposed change be applied to them as well? >>> transaction_t >>> - t_chp_stats: j_checkpoint_sem > > Where do we read t_chp_stats outside of a lock? j_list_lock seems to be > used pretty consistently there. Except perhaps > __jbd2_journal_remove_checkpoint() but there we know we are already the > only ones touching the transaction and thus its statistics. > I'm sorry. That's my mistake. There's no access without a lock. >>> jbd2_inode: >>> - i_list: j_list_lock > > And here as well. I would not complicate the locking description unless we > really have places that access these fields without locks... > Same here. - Alex > Honza > -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2021-02-09 9:58 ` Alexander Lochmann @ 2021-02-09 12:00 ` Jan Kara 2021-02-09 13:47 ` Alexander Lochmann 0 siblings, 1 reply; 13+ messages in thread From: Jan Kara @ 2021-02-09 12:00 UTC (permalink / raw) To: Alexander Lochmann Cc: Jan Kara, tytso, Jan Kara, Horst Schirmeier, linux-ext4 On Tue 09-02-21 10:58:48, Alexander Lochmann wrote: > > > On 08.02.21 16:27, Jan Kara wrote: > > Hi Alexander! > > > > On Fri 05-02-21 16:31:54, Alexander Lochmann wrote: > > > have you had the chance to review our results? > > > > It fell through the cracks I guess. Thanks for pinging. Let me have a look. > > > > > On 15.10.20 15:56, Alexander Lochmann wrote: > > > > Hi folks, > > > > > > > > when comparing our generated locking documentation with the current > > > > documentation > > > > located in include/linux/jbd2.h, I found some inconsistencies. (Our > > > > approach: https://dl.acm.org/doi/10.1145/3302424.3303948) > > > > According to the official documentation, the following members should be > > > > read using a lock: > > > > journal_t > > > > - j_flags: j_state_lock > > > > - j_barrier_count: j_state_lock > > > > - j_running_transaction: j_state_lock > > > > - j_commit_sequence: j_state_lock > > > > - j_commit_request: j_state_lock > > > > transactiont_t > > > > - t_nr_buffers: j_list_lock > > > > - t_buffers: j_list_lock > > > > - t_reserved_list: j_list_lock > > > > - t_shadow_list: j_list_lock > > > > jbd2_inode > > > > - i_transaction: j_list_lock > > > > - i_next_transaction: j_list_lock > > > > - i_flags: j_list_lock > > > > - i_dirty_start: j_list_lock > > > > - i_dirty_start: j_list_lock > > > > > > > > However, our results say that no locks are needed at all for *reading* > > > > those members. > > > > From what I know, it is common wisdom that word-sized data types can be > > > > read without any lock in the Linux kernel. > > > > Yes, although in last year, people try to convert these unlocked reads to > > READ_ONCE() or similar as otherwise the compiler is apparently allowed to > > generate code which is not safe. But that's a different story. > Is this ongoing work? Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot ;). > Using such a macro would a) make our work much easier as we can instrument > them, and b) would tell less experienced developers that no locking is > needed. Yes, I agree that it has some benefit for documentation and automatic checkers as well. OTOH code readability is sometimes hurt by this... > Does the usage of READ_ONCE() imply that no lock is needed? No, but it does indicate there's something unusual happening with the variable - usually that variable write can race with this read. > Otherwise, one could introduce another macro for jbd2, such as #define > READ_UNLOCKED() READ_ONCE(), which is more precise. Well, yes, but OTOH special macros for small subsystems like this are making more harm than good in terms of readability since people have to lookup what exactly they mean anyway. > Also note > > that although reading that particular word may be safe without any other > > locks, the lock still may be needed to safely interpret the value in the > > context of other fetched values (e.g., due to consistency among multiple > > structure members). > Just a side quest: Do you have an example for such a situation? Definitely. The simplest case is: You can fetch journal->j_running_transaction pointer any time without any problem. But you can *dereference* it only if you hold the j_state_lock while fetching the pointer and dereferencing it. > So sometimes requiring the lock is just the least > > problematic solution - there's always the tradeoff between the speed and > > simplicity. > > > > > > All of the above members have word size, i.e., int, long, or ptr. > > > > Is it therefore safe to split the locking documentation as follows? > > > > @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] > > > > I've checked the code and we usually use unlocked reads for quick, possibly > > racy checks and if they indicate we may need to do something then take the > > lock and do a reliable check. This is quite common pattern, not sure how to > > best document this. Maybe like [j_state_lock, no lock for quick racy checks]? > > > Yeah, I'm fine with that. Does this rule apply for the other members of > journal_t (and transaction_t?) listed above? Yes. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2021-02-09 12:00 ` Jan Kara @ 2021-02-09 13:47 ` Alexander Lochmann 2021-02-09 16:48 ` Jan Kara 0 siblings, 1 reply; 13+ messages in thread From: Alexander Lochmann @ 2021-02-09 13:47 UTC (permalink / raw) To: Jan Kara; +Cc: tytso, Jan Kara, Horst Schirmeier, linux-ext4 [-- Attachment #1.1: Type: text/plain, Size: 3283 bytes --] On 09.02.21 13:00, Jan Kara wrote: >>> Yes, although in last year, people try to convert these unlocked reads to >>> READ_ONCE() or similar as otherwise the compiler is apparently allowed to >>> generate code which is not safe. But that's a different story. >> Is this ongoing work? > > Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot > ;). > >> Using such a macro would a) make our work much easier as we can instrument >> them, and b) would tell less experienced developers that no locking is >> needed. > > Yes, I agree that it has some benefit for documentation and automatic > checkers as well. OTOH code readability is sometimes hurt by this... > >> Does the usage of READ_ONCE() imply that no lock is needed? > > No, but it does indicate there's something unusual happening with the > variable - usually that variable write can race with this read. > >> Otherwise, one could introduce another macro for jbd2, such as #define >> READ_UNLOCKED() READ_ONCE(), which is more precise. > > Well, yes, but OTOH special macros for small subsystems like this are making > more harm than good in terms of readability since people have to lookup > what exactly they mean anyway. So the only option left would be a global macro such as READ_ONCE() I guess. How hard would it be to establish such a global notation? It would make things a lot easier for LockDoc, because we can instrument such a macro, and therefore can annotate those accesses.> > Definitely. The simplest case is: You can fetch > journal->j_running_transaction pointer any time without any problem. But > you can *dereference* it only if you hold the j_state_lock while fetching the > pointer and dereferencing it. Thx. > >> So sometimes requiring the lock is just the least >>> problematic solution - there's always the tradeoff between the speed and >>> simplicity. >>> >>>>> All of the above members have word size, i.e., int, long, or ptr. >>>>> Is it therefore safe to split the locking documentation as follows? >>>>> @j_flags: General journaling state flags [r:nolocks, w:j_state_lock] >>> >>> I've checked the code and we usually use unlocked reads for quick, possibly >>> racy checks and if they indicate we may need to do something then take the >>> lock and do a reliable check. This is quite common pattern, not sure how to >>> best document this. Maybe like [j_state_lock, no lock for quick racy checks]? >>> >> Yeah, I'm fine with that. Does this rule apply for the other members of >> journal_t (and transaction_t?) listed above? > > Yes. Thx. I'll submit a patch for those elements. For now, this will improve LockDoc's results as we can add "no locks needed" to our config for j_flags. We check whether the observed accesses match the documented locking rules. LockDoc will accept both results "j_list_lock" and "no locks needed" for reading j_flags. However, real faulty unlocked accesses will be concealed. :-( - Alex > > Honza > -- Technische Universität Dortmund Alexander Lochmann PGP key: 0xBC3EF6FD Otto-Hahn-Str. 16 phone: +49.231.7556141 D-44227 Dortmund fax: +49.231.7556116 http://ess.cs.tu-dortmund.de/Staff/al [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC] Fine-grained locking documentation for jbd2 data structures 2021-02-09 13:47 ` Alexander Lochmann @ 2021-02-09 16:48 ` Jan Kara 0 siblings, 0 replies; 13+ messages in thread From: Jan Kara @ 2021-02-09 16:48 UTC (permalink / raw) To: Alexander Lochmann Cc: Jan Kara, tytso, Jan Kara, Horst Schirmeier, linux-ext4 On Tue 09-02-21 14:47:28, Alexander Lochmann wrote: > On 09.02.21 13:00, Jan Kara wrote: > > > > Yes, although in last year, people try to convert these unlocked reads to > > > > READ_ONCE() or similar as otherwise the compiler is apparently allowed to > > > > generate code which is not safe. But that's a different story. > > > Is this ongoing work? > > > > Yes, in a way. It's mostly prompted by KCSAN warnings generated by syzbot > > ;). > > > > > Using such a macro would a) make our work much easier as we can instrument > > > them, and b) would tell less experienced developers that no locking is > > > needed. > > > > Yes, I agree that it has some benefit for documentation and automatic > > checkers as well. OTOH code readability is sometimes hurt by this... > > > > > Does the usage of READ_ONCE() imply that no lock is needed? > > > > No, but it does indicate there's something unusual happening with the > > variable - usually that variable write can race with this read. > > > > > Otherwise, one could introduce another macro for jbd2, such as #define > > > READ_UNLOCKED() READ_ONCE(), which is more precise. > > > > Well, yes, but OTOH special macros for small subsystems like this are > > making more harm than good in terms of readability since people have to > > lookup what exactly they mean anyway. > > So the only option left would be a global macro such as READ_ONCE() I guess. > How hard would it be to establish such a global notation? > It would make things a lot easier for LockDoc, because we can instrument > such a macro, and therefore can annotate those accesses.> Well, READ_ONCE() macro already exists in the kernel and is used in quite some places. Generally, there's concensus that newly added unlocked accesses should use that macro. But the amount of already existing unlocked accesses in the kernel is large and the problem is mostly theoretical / for machine checkers so there's some resistance to the churn generated by global search-and-replace approach. But as I said we are moving in that direction and eventually get there. Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-09 16:51 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-08 8:35 [PATCH v3] Updated locking documentation for transaction_t Alexander Lochmann 2019-06-20 20:45 ` Alexander Lochmann 2020-10-15 13:26 ` Alexander Lochmann 2020-12-03 14:04 ` Theodore Y. Ts'o 2020-12-03 14:38 ` Alexander Lochmann 2020-12-03 20:39 ` Theodore Y. Ts'o 2020-10-15 13:56 ` [RFC] Fine-grained locking documentation for jbd2 data structures Alexander Lochmann 2021-02-05 15:31 ` Alexander Lochmann 2021-02-08 15:27 ` Jan Kara 2021-02-09 9:58 ` Alexander Lochmann 2021-02-09 12:00 ` Jan Kara 2021-02-09 13:47 ` Alexander Lochmann 2021-02-09 16:48 ` Jan Kara
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.