On Tue, Apr 09, 2019 at 11:47:50AM +0300, Nikolay Borisov wrote:
[...]
> Overall looks good and not nearly as ugly as I expected so the csum tree
> is not _THAT_ cumbersome to work with after all :). However, do you
> intend to submit tests with files with specific patterns to ensure we do
> not regress? Also I have some minor comments below but they are mostly
> cosmetic so:
Yes test are planned, once I've figured out how to properly do this with
btrfs-progs test framework.
[...]
> In cmds-inspect-dump-super.c there is a function 'load_and_dump_sb', IMO
> it will make sense to split it into load_sb and dump/print_sb. That way
> the former could be made into a library function and moved to utils.c
> this will enable you to query the size of the csum dynamically.
Ah this is something I've been looking for. Thanks for the pointer will do it.
>
>
> > + int ret, i, j;
> > + u64 needle = phys;
> > + u64 pending_csums = extent_csums;
>
> Perhahps pending_csums could be renamed to something more descriptive e.g:
>
> pending_csum_count
>
> OTOH the plural form slightly hints at the possible usage but while I
> was reviewing the code I had to scroll up to be sure what the variable
> holds.
Sure, no problem.
>
> > +
> > + memset(buf, 0, sizeof(buf));
> > + search = (struct btrfs_ioctl_search_args_v2 *)buf;
> > + sk = &search->key;
> > +
> > +again:
> > + if (debug)
> > + printf(
> > +"Looking up checksums for extent at physial offset: %llu (searching at %llu), looking for %llu csums\n",
> > + phys, needle, pending_csums);
> > +
> > + sk->tree_id = BTRFS_CSUM_TREE_OBJECTID;
> > + sk->min_objectid = BTRFS_EXTENT_CSUM_OBJECTID;
> > + sk->max_objectid = BTRFS_EXTENT_CSUM_OBJECTID;
> > + sk->max_type = BTRFS_EXTENT_CSUM_KEY;
> > + sk->min_type = BTRFS_EXTENT_CSUM_KEY;
> > + sk->min_offset = needle;
> > + sk->max_offset = (u64)-1;
> > + sk->max_transid = (u64)-1;
> > + sk->nr_items = 1;
> > + search->buf_size = bufsz - sizeof(*search);
> > +
> > + ret = ioctl(fd, BTRFS_IOC_TREE_SEARCH_V2, search);
> > + if (ret < 0)
> > + return ret;
> > +> + if (sk->nr_items == 0) {
>
> I'd like a comment here that this is a heuristics
OK.
>
> > + needle -= 4096;
> > + goto again;
> > + }
>
> nit: My personal preference is to always use the idiomatic constructs
> that a language gives to developers. I know it will increase the
> indentation level but a do {} while(!sk->nr_items) feels more correct
> than constant abuse of labels (and in the btrfs code base we are grave
> offenders in that regard. This is only my opinion so if David feels it'
> better to leave the label-based construct I'm not going to argue.
I personally prefer labels for cases like this, where it's the less likely
case. Plus it saves us one level of indentation. But I do what ever is
preferred here.
[...]
> > + if (csums_in_item > pending_csums)
> > + csums_in_item = pending_csums;
>
> nit: csums_in_item = max(csums_in_item, pending_csums);
Args, yeah. Stupid me.
[...]
> > + tmp = realloc(fiemap, sizeof(*fiemap) + ext_size);
> > + if (!tmp)
> > + goto free_fiemap;
>
> That works but if a file has A LOT OF extents then this could
> potentially trigger a very large allocation. A different strategy is to
> have a fixed number of extents and read the whole file in a loop. This
> could of course be added later. Just mentioning it.
Yes I saw xfs_io's fiemap command does use batches, but we're in user-space
and with memory overcommit per default I don't think doing large allocations
in a debug/diagnostics tool hurts too much. I can easily transform it to
batched allocations + queries though if you want though.
Thanks for the review,
Johannes
--
Johannes Thumshirn SUSE Labs Filesystems
jthumshirn@xxxxxxx +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
