On Fri, Feb 10, 2012 at 3:33 AM, Jan Schmidt <list.btrfs@xxxxxxxxxxxxx> wrote:
> Hi Mitch,
>
> having this patch on the list is a good idea. I've two remarks, just in
> case it will be included sometimes:
>
> On 09.02.2012 22:38, Mitch Harder wrote:
>> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
>> index fe4cd0f..31b717f 100644
>> --- a/fs/btrfs/delayed-inode.c
>> +++ b/fs/btrfs/delayed-inode.c
>> @@ -656,8 +656,18 @@ static int btrfs_delayed_inode_reserve_metadata(
>> * EAGAIN to make us stop the transaction we have, so return
>> * ENOSPC instead so that btrfs_dirty_inode knows what to do.
>> */
>> +#ifdef BTRFS_DEBUG_ENOSPC
>> + if (unlikely(ret == -EAGAIN)) {
>> + ret = -ENOSPC;
>> + if (printk_ratelimit())
>
> From linux/printk.h:
> /*
> * Please don't use printk_ratelimit(), because it shares ratelimiting state
> * with all other unrelated printk_ratelimit() callsites. Instead use
> * printk_ratelimited() or plain old __ratelimit().
> */
>
> printk_ratelimited() seems the right choice, here.
>
OK, I'll change those. Apparently, I was looking at some out-of-date examples.
>> + printk(KERN_WARNING
>> + "btrfs: ENOSPC set in "
>> + "btrfs_delayed_inode_reserve_metadata\n");
>> + }
>> +#else
>> if (ret == -EAGAIN)
>> ret = -ENOSPC;
>> +#endif
>
> I don't like the #if placements throughout the patch. Copying all the
> conditions is error prone (especially when changes are made later on).
>
> I'd rather leave all the conditions as they stand (possibly adding the
> unlikely() macro if you wish). Same for the following return statement
> or assignment. Simply put printk_ratelimited() into the #if ... #endif.
>
The reason my #if macros became so large is that I often needed to
replace a simple if statement like:
if (condition)
ret = -ENOSPC;
with a more complex if statement that required brackets
if (condition) {
printk_ratelimited();
ret = -ENOSPC;
}
I injected the 'unlikely' macro to minimize the impact of the expanded
if statements. I've noticed that some Btrfs errors depend on speed
(or possibly lack thereof).
I'll review my #if macros, and see if I can tighten them up.
--
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
