Re: [PATCH] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log

[Jan Schmidt <list.btrfs@jan-o-sch.net>]

On Wed, April 24, 2013 at 15:04 (+0200), Josef Bacik wrote:
> On Tue, Apr 23, 2013 at 12:00:27PM -0600, Jan Schmidt wrote:
>> Sequence numbers for delayed refs have been introduced in the first version
>> of the qgroup patch set. To solve the problem of find_all_roots on a busy
>> file system, the tree mod log was introduced. The sequence numbers for that
>> were simply shared between those two users.
>>
>> However, at one point in qgroup's quota accounting, there's a statement
>> accessing the previous sequence number, that's still just doing (seq - 1)
>> just as it had to in the very first version.
>>
>> To satisfy that requirement, this patch makes the sequence number counter 64
>> bit and splits it into a major part (used for qgroup sequence number
>> counting) and a minor part (incremented for each tree modification in the
>> log). This enables us to go exactly one major step backwards, as required
>> for qgroups, while still incrementing the sequence counter for tree mod log
>> insertions to keep track of their order. Keeping them in a single variable
>> means there's no need to change all the code dealing with comparisons of two
>> sequence numbers.
>>
>> The sequence number is reset to 0 on commit (not new in this patch), which
>> ensures we won't overflow the two 32 bit counters.
>>
>> Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs
>> from the tree mod log code may happen.
>>
>> Signed-off-by: Jan Schmidt <list.btrfs@jan-o-sch.net>
>> ---
>>  fs/btrfs/ctree.c       |   36 +++++++++++++++++++++++++++++++++---
>>  fs/btrfs/ctree.h       |    7 ++-----
>>  fs/btrfs/delayed-ref.c |    6 ++++--
>>  fs/btrfs/disk-io.c     |    2 +-
>>  fs/btrfs/extent-tree.c |    5 +++--
>>  fs/btrfs/qgroup.c      |   13 ++++++++-----
>>  fs/btrfs/transaction.c |    2 +-
>>  7 files changed, 52 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
>> index 566d99b..b74136e 100644
>> --- a/fs/btrfs/ctree.c
>> +++ b/fs/btrfs/ctree.c
>> @@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct btrfs_fs_info *fs_info)
>>  }
>>  
>>  /*
>> + * increment the upper half of tree_mod_seq, set lower half zero
>> + *
>> + * must be called with fs_info->tree_mod_seq_lock held
>> + */
>> +static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info *fs_info)
>> +{
>> +	u64 seq = atomic64_read(&fs_info->tree_mod_seq);
>> +	seq &= 0xffffffff00000000ull;
>> +	seq += 1ull << 32;
>> +	atomic64_set(&fs_info->tree_mod_seq, seq);
>> +	return seq;
>> +}
> 
> This isn't going to work, you read in the value, inc it and then set the new
> value.  If somebody comes in and inc's in between the read and the sync, like
> btrfs_inc_tree_mod_seq_minor could do when you call tree_mod_alloc, you'll end
> up losing the minor update.  Thanks,

I don't think I'll lose it. The minor update is made and returned to the one who
needs it, that number can still be used. There is no guarantee for two
concurrent modifications to which major a minor number belongs, though.

Thanks,
-Jan