On 2018年03月22日 22:20, Nikolay Borisov wrote: > > > On 22.03.2018 16:17, Qu Wenruo wrote: >> >> >> On 2018年03月22日 22:00, David Sterba wrote: >>> On Thu, Mar 22, 2018 at 09:53:46PM +0800, Qu Wenruo wrote: >>>>>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c >>>>>> index 26484648d090..3866b8ab20f1 100644 >>>>>> --- a/fs/btrfs/backref.c >>>>>> +++ b/fs/btrfs/backref.c >>>>>> @@ -738,7 +738,8 @@ static int add_missing_keys(struct btrfs_fs_info >>>>>> *fs_info, >>>>>> BUG_ON(ref->key_for_search.type); >>>>>> BUG_ON(!ref->wanted_disk_byte); >>>>>> >>>>>> - eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0); >>>>>> + eb = read_tree_block(fs_info, ref->wanted_disk_byte, 0, >>>>>> NULL, >>>>>> + 0); >>>>> >>>>> Please add 2nd function that will take the extended parameters and >>>>> keep read_tree_block as is. >>>> >>>> So for any new caller of read_tree_block(), reviewer is the last person >>>> to info the author to use these parameters for safety check? >>>> >>>> And in fact, the old function should be avoid if possible, I think the >>>> new parameters act as a pretty good sign to make any caller double think >>>> about this. >>> >>> I saw half of the new parameters were just 0, NULL, so this looks like a >>> lot of code churn and I haven't looked closer if there's a chance to >>> fill the parameters in all callsites. So if it's a matter of adding them >>> incrementally then fine. >>> >> I'm afraid some of the call sites (ones I left with NULL, 0) are unable >> to pass the new parameters by its nature. >> >> Such callers include: >> 1) Tree root >> Just @bytenr and @gen from ROOT_ITEM. No @first_key. >> >> 2) Backref walker for FULL_BACKREF >> Only parent bytenr, no extra info on @first_key. >> >> But despite of such call sites, every top-down reader should grab first >> key and level. (And so I did in the patch). >> >> BTW, about half of the read_tree_block() callers are using the new >> parameters. >> So a new function seems a little embarrassing here. > > > Is it possible to centralise those checks in the read tree verifier, > rather than sprinkling them around the code?
The problem is, tree checker can only handle things *inside* a leaf/node, nothing can go beyond leaf/node boundary. And for current check, we need a top-down pointer (nodeptr, which has bytenr, generation, first key along with the level) to do the verification, so that's the reason we can't put it into tree-checker. (Sorry I forgot to add this explanation, and I didn't find better solution) Thanks, Qu > >> >> Thanks, >> Qu >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
signature.asc
Description: OpenPGP digital signature
