Re: [PATCH 1/2] Btrfs: use rcu to protect device->name V2

[Josef Bacik <josef@redhat.com>]

On Wed, Jun 13, 2012 at 03:49:07PM +0200, Stefan Behrens wrote:
> On Wed, 13 Jun 2012 09:14:27 -0400, Josef Bacik wrote:
> > On Wed, Jun 13, 2012 at 12:35:26AM +0200, David Sterba wrote:
> >> On Tue, Jun 12, 2012 at 03:50:41PM -0400, Josef Bacik wrote:
> >>> @@ -4694,8 +4716,11 @@ int btrfs_init_dev_stats(struct btrfs_fs_info *fs_info)
> >>>  		key.offset = device->devid;
> >>>  		ret = btrfs_search_slot(NULL, dev_root, &key, path, 0, 0);
> >>>  		if (ret) {
> >>> -			printk(KERN_WARNING "btrfs: no dev_stats entry found for device %s (devid %llu) (OK on first mount after mkfs)\n",
> >>> -			       device->name, (unsigned long long)device->devid);
> >>> +			printk_in_rcu(KERN_WARNING "btrfs: no dev_stats entry "
> >>> +				      "found for device %s (devid %llu) (OK on"
> >>> +				      " first mount after mkfs)\n",
> >>
> >> breaking printk strings hurts when grepping for a message
> >>
> >>> +				      rcu_str_deref(device->name),
> >>> +				      (unsigned long long)device->devid);
> >>>  			__btrfs_reset_dev_stats(device);
> >>>  			device->dev_stats_valid = 1;
> >>>  			btrfs_release_path(path);
> >>> @@ -4747,8 +4772,9 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
> >>>  	BUG_ON(!path);
> >>>  	ret = btrfs_search_slot(trans, dev_root, &key, path, -1, 1);
> >>>  	if (ret < 0) {
> >>> -		printk(KERN_WARNING "btrfs: error %d while searching for dev_stats item for device %s!\n",
> >>> -		       ret, device->name);
> >>> +		printk_in_rcu(KERN_WARNING "btrfs: error %d while searching "
> >>> +			      "for dev_stats item for device %s!\n", ret,
> >>
> >> and here as well
> >>
> >>> +			      rcu_str_deref(device->name));
> >>>  		goto out;
> >>>  	}
> >>>  
> >>> @@ -4757,8 +4783,9 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
> >>>  		/* need to delete old one and insert a new one */
> >>>  		ret = btrfs_del_item(trans, dev_root, path);
> >>>  		if (ret != 0) {
> >>> -			printk(KERN_WARNING "btrfs: delete too small dev_stats item for device %s failed %d!\n",
> >>> -			       device->name, ret);
> >>> +			printk_in_rcu(KERN_WARNING "btrfs: delete too small "
> >>> +				      "dev_stats item for device %s failed %d!\n",
> >>
> >> here
> >>
> >>> +				      rcu_str_deref(device->name), ret);
> >>>  			goto out;
> >>>  		}
> >>>  		ret = 1;
> >>> @@ -4770,8 +4797,9 @@ static int update_dev_stat_item(struct btrfs_trans_handle *trans,
> >>>  		ret = btrfs_insert_empty_item(trans, dev_root, path,
> >>>  					      &key, sizeof(*ptr));
> >>>  		if (ret < 0) {
> >>> -			printk(KERN_WARNING "btrfs: insert dev_stats item for device %s failed %d!\n",
> >>> -			       device->name, ret);
> >>> +			printk_in_rcu(KERN_WARNING "btrfs: insert dev_stats "
> >>> +				      "item for device %s failed %d!\n",
> >>
> >> here
> >>
> >>> +				      rcu_str_deref(device->name), ret);
> >>>  			goto out;
> >>>  		}
> >>>  	}
> >>
> >> mostly minor things, but please fix them.
> >>
> > 
> > I'm breaking them for the 80 char limit, it happens for all long messages, we're
> > all used to it.  I'll fix up the other things.  Thanks,
> > 
> > Josef
> 
> The last sentence of chapter 2 of Documentation/CodingStyle is quite
> unambiguous. Here is the full quote of that chapter:
> 
>                 Chapter 2: Breaking long lines and strings
> 
> Coding style is all about readability and maintainability using commonly
> available tools.
> 
> The limit on the length of lines is 80 columns and this is a strongly
> preferred limit.
> 
> Statements longer than 80 columns will be broken into sensible chunks,
> unless
> exceeding 80 columns significantly increases readability and does not hide
> information. Descendants are always substantially shorter than the
> parent and
> are placed substantially to the right. The same applies to function headers
> with a long argument list. However, never break user-visible strings such as
> printk messages, because that breaks the ability to grep for them.

Ah never seen that part of it, I will leave them alone then.  Thanks,

Josef