Re: [PATCH v3 3/5] btrfs: Detect unbalanced tree with empty leaf before crashing btree operations

On 8/11/20 8:29 PM, Qu Wenruo wrote:

On 2020/8/12 上午8:23, Josef Bacik wrote:

quoted

On 8/11/20 7:04 PM, Qu Wenruo wrote:

quoted

[...]

quoted

quoted

quoted

Which I assume is the problem?  The generation is 19, is that >
last_trans_committed?  Seems like this check just needs to be moved
lower, right?  Thanks,

Nope, that generation 19 is valid. That fs has a higher generation, so
that's completely valid.

The generation 19 is there because there is another csum leaf whose
generation is 19.

Then this patch does nothing, because we already have this check lower,
so how exactly did it make the panic go away?  Thanks,

Josef

Sorry, I don't get your point.

The generation 19 isn't larger than last_trans_committed, so that check
has nothing to do with this case.

And then it goes to the header_nritems() check, which is 0, and with
first_key present, which is invalid and we error out, rejecting the
corrupted leaf.

What's the problem then?

         /* We have @first_key, so this @eb must have at least one item */
         if (btrfs_header_nritems(eb) == 0) {
                 btrfs_err(fs_info,
                 "invalid tree nritems, bytenr=%llu nritems=0 expect >0",
                           eb->start);
                 WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
                 return -EUCLEAN;
         }

         /*
          * For live tree block (new tree blocks in current transaction),
          * we need proper lock context to avoid race, which is 
impossible here.
          * So we only checks tree blocks which is read from disk, whose
          * generation <= fs_info->last_trans_committed.
          */
         if (btrfs_header_generation(eb) > fs_info->last_trans_committed)
                 return 0;

         /* We have @first_key, so this @eb must have at least one item */
         if (btrfs_header_nritems(eb) == 0) {
                 btrfs_err(fs_info,
                 "invalid tree nritems, bytenr=%llu nritems=0 expect >0",
                           eb->start);
                 WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
                 return -EUCLEAN;
         }


This is the code, you have the exact same check above the header 
generation thing, and that's not the problem, so I don't understand why 
you have added this check.

Josef