Lots of quotation removed. All removed comments accepted.
On 16.06.2011 23:26, David Sterba wrote:
> On Mon, Jun 13, 2011 at 09:10:36PM +0200, Jan Schmidt wrote:
>> +struct scrub_warning {
>> + struct btrfs_path *path;
>> + u64 extent_item_size;
>> + char *scratch_buf;
>> + char *msg_buf;
>> + const char *errstr;
>> + u64 sector;
>
> better named 'physical', like in scrub_bio
As I save a sector there and not a physical address, I still prefer
"sector". But as a compromise, I'll change the type to sector_t instead.
>> +static void scrub_print_warning(const char *errstr, struct scrub_bio *sbio,
>> + int ix)
>
> please rename 'ix' to eg 'idx', more readable
I'd like to stay consistent with the rest of the file, using ix at
similar opportunities. And besides: I like ix more than idx :-)
>> +{
>> + struct btrfs_device *dev = sbio->sdev->dev;
>> + struct btrfs_fs_info *fs_info = dev->dev_root->fs_info;
>> + struct btrfs_path *path;
>> + struct btrfs_key found_key;
>> + struct extent_buffer *eb;
>> + struct btrfs_extent_item *ei;
>> + struct scrub_warning swarn;
>> + u32 item_size;
>> + int ret;
>> + u64 ref_root;
>
> i'm not able to understand what a u64 value describing a root means, seeing it
> here or in the printk message. it's returned as
>
> *dest_root = btrfs_extent_inline_ref_offset(eb, eiref);
>
> can you please explain it to me?
Have a look at the BTRFS_*_TREE_OBJECTID #defines in ctree.h.
btrfs-debug-tree prints them as
tree block backref root 2
>> + u64 ref_level;
>
> int is enough, as noted elsewhere
>
>> + unsigned long ptr = 0;
>> + static const int bufsize = 4096;
>
> drop static, this would allocate a memory cell to hold the value 4096, while
> without it, it's just a named constant and this will make the compiler happy
>
>> + loff_t extent_item_offset;
>
> loff_t is a signed long long, do you really need it?
Seems I don't. I was looking around how I came to use loff_t in the
first place, but couldn't find a good reason. I'm replacing it with u64,
which makes more sense anyway, since the loff_t parameter I'm passing
around is the result of a subtraction of two u64s.
>> +
>> + path = btrfs_alloc_path();
>> +
>> + swarn.scratch_buf = kmalloc(bufsize, GFP_NOFS);
>> + swarn.msg_buf = kmalloc(bufsize, GFP_NOFS);
>> + swarn.sector = (sbio->physical + ix * PAGE_SIZE) >> 9;
>> + swarn.logical = sbio->logical + ix * PAGE_SIZE;
>> + swarn.errstr = errstr;
>> + swarn.dev = dev;
>> + swarn.msg_bufsize = bufsize;
>> + swarn.scratch_bufsize = bufsize;
>> +
>> + if (!path || !swarn.scratch_buf || !swarn.msg_buf)
>> + goto out;
>> +
>> + ret = data_extent_from_logical(fs_info->extent_root,
>> + swarn.logical, path, &found_key);
>> + if (ret < 0)
>> + goto out;
>> +
>> + extent_item_offset = swarn.logical - found_key.objectid;
>> + swarn.extent_item_size = found_key.offset;
>> +
>> + eb = path->nodes[0];
>> + ei = btrfs_item_ptr(eb, path->slots[0], struct btrfs_extent_item);
>> + item_size = btrfs_item_size_nr(eb, path->slots[0]);
>> +
>> + btrfs_release_path(path);
>
> is it safe to release the path here? it's used in the else branch below
Yes, and it's even required, since iterate_extent_inodes() wants a
clean, usable path for itself. To make that more clear, I'll move the
call to btrfs_release_path inside the else block.
>> +
>> + if (ret) {
>> + ret = tree_backref_for_extent(&ptr, eb, ei, item_size,
>> + &ref_root, &ref_level);
>> + printk(KERN_WARNING "%s at logical %llu on dev %s, "
>> + "sector %llu: metadata %s (level %llu) in tree %llu\n",
>> + errstr, swarn.logical, dev->name, swarn.sector,
>> + ref_level ? "node" : "leaf", ret < 0 ? -1 : ref_level,
>> + ret < 0 ? -1 : ref_root);
>> + } else {
>> + swarn.path = path;
>> + iterate_extent_inodes(eb, ei, extent_item_offset, item_size,
>> + scrub_print_warning_inode, &swarn);
>> + }
>> +
>> +out:
>> + btrfs_free_path(path);
>> + kfree(swarn.scratch_buf);
>> + kfree(swarn.msg_buf);
>> +}
>> +
>> /*
>> * scrub_recheck_error gets called when either verification of the page
>> * failed or the bio failed to read, e.g. with EIO. In the latter case,
>> @@ -218,6 +347,11 @@ static int scrub_recheck_error(struct scrub_bio *sbio, int ix)
>> if (scrub_fixup_check(sbio, ix) == 0)
>> return 0;
>> }
>> + if (printk_ratelimit())
>
> please don't use printk_ratelimit; as a simple printk_ratelimited is not
> applicable here, we have to use __ratelimit: (include/linux/printk.h)
>
> static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
> DEFAULT_RATELIMIT_BURST);
>
> if (__ratelimit(&_rs))
Modified as you suggested, I only included linux/ratelimit.h instead.
Thanks!
Jan
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
