[Devel] [PATCH 2/3] target: allow to set a blkio cgroup for a backstore

[Cyrill Gorcunov]

On Mon, May 07, 2018 at 11:35:35AM -0700, Andrei Vagin wrote:
> On Sat, May 05, 2018 at 09:12:02AM +0300, Cyrill Gorcunov wrote:
> > On Sat, May 05, 2018 at 01:42:36AM +0300, Andrei Vagin wrote:
> > > The Block I/O (blkio) subsystem controls and monitors access to I/O on
> > > block devices by tasks in cgroups. With the introduced changes, a
> > > backstore will be like a task in a specified group.
> > > 
> > > One of interesting feature is an ability to set limits on a number of
> > > I/O operations and bytes per seconds.
> > > 
> > > A new attribute is added for backstores, it is called blkio_cgroup.
> > > 
> > > If we write 1 to the attribute file, a blkio cgroup from the current
> > > process is attached to the backstore.
> > > 
> > > If we write 0 to the attribute file, a current group will be detached
> > > from the backstore.
> > > 
> > > When we know a blkio cgroup the only thing, what we need to do to make it
> > > work, is to set this group for bio-s.
> > > 
> > > How to use:
> > >  # Create a test backstore
> > > $ targetcli
> > > targetcli shell version 2.1.fb46
> > > Copyright 2011-2013 by Datera, Inc and others.
> > > /backstores/block> create dev=/dev/loop0 loop0
> > > Created block storage object loop0 using /dev/loop0.
> > > /backstores/block> cd /loopback
> > > /loopback> create
> > > Created target naa.50014056fd3f341c.
> > > /loopback> cd naa.50014056fd3f341c/luns
> > > /loopback/naa...fd3f341c/luns> create /backstores/block/loop0
> > > Created LUN 0.
> > > /loopback/naa...fd3f341c/luns> exit
> > > 
> > >  # Create a test cgroup and set it to a test backstore
> > > $ CG_PATH=/sys/fs/cgroup/blkio/test
> > > $ BS_PATH=/sys/kernel/config/target/core/iblock_0/loop0/attrib/blkio_cgroup
> > > $ mkdir -p $CG_PATH
> > > $ bash -c "echo 0 > $CG_PATH/tasks && echo 1 > $BS_PATH"
> > > $ cat $BS_PATH
> > > /test
> > > 
> > >  # Set 6 MB/sec for the backstore
> > > $ echo "7:0 6291456" > $CG_PATH/blkio.throttle.read_bps_device
> > > 
> > >  # Check that everything work as expected
> > > $ dd if=/dev/sda of=/dev/null iflag=direct bs=1M count=100
> > > 100+0 records in
> > > 100+0 records out
> > > 104857600 bytes (105 MB, 100 MiB) copied, 16.6958 s, 6.3 MB/s
> > > 
> > > Signed-off-by: Andrei Vagin <avagin at openvz.org>
> > > ---
> > >  drivers/target/target_core_device.c  | 58 ++++++++++++++++++++++++++++++++++++
> > >  drivers/target/target_core_iblock.c  | 38 +++++++++++++++++++++++
> > >  include/target/target_core_backend.h |  4 +++
> > >  include/target/target_core_base.h    |  2 ++
> > >  4 files changed, 102 insertions(+)
> > > 
> > > diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> > > index cb17aeb..fb1e940 100644
> > > --- a/drivers/target/target_core_device.c
> > > +++ b/drivers/target/target_core_device.c
> > > @@ -831,6 +831,58 @@ int se_dev_set_emulate_fua_read(struct se_device *dev, int flag)
> > >  }
> > >  EXPORT_SYMBOL(se_dev_set_emulate_fua_read);
> > >  
> > > +ssize_t se_dev_blkio_cgroup_show(struct se_device *dev, char *page)
> > > +{
> > > +	int rb;
> > > +
> > > +	read_lock(&dev->dev_attrib_lock);
> > > +	if (dev->dev_attrib.blk_css) {
> > > +		rb = cgroup_path(dev->dev_attrib.blk_css->cgroup,
> > > +						page, PAGE_SIZE - 1);
> > > +		if (rb == 0)
> > > +			rb = strlen(page);
> > 
> > Maybe it would worth to use strnlen(page, PAGE_SIZE - 2); here?
> 
> cgroup_path returns a null terminated string, so I don't understand a
> reason for these changes.

1) To be on a safe side but surely not mandatory
2) Why don't you check for non zero return code?