[stgt] [PATCH] LBPPBE: when opening the backing file, check the blocksize of the underlying filesystem and set lbppbe automatically.

[ronnie sahlberg]

On Wed, Apr 18, 2012 at 9:30 PM, FUJITA Tomonori
<fujita.tomonori at lab.ntt.co.jp> wrote:
> On Wed, 18 Apr 2012 19:26:47 +1000
> Ronnie Sahlberg <ronniesahlberg at gmail.com> wrote:
>
>> Unless the user has set an explicit value through tgtadm in which case we leave the system using the value the user has set.
>>
>> Signed-off-by: Ronnie Sahlberg <ronniesahlberg at gmail.com>
>> ---
>>  doc/targets.conf.5.xml |    7 ++++++-
>>  usr/bs_aio.c           |   19 ++++++++++++-------
>>  usr/bs_rdwr.c          |   10 ++++++++--
>>  usr/bs_ssc.c           |    2 +-
>>  usr/spc.c              |    1 +
>>  usr/target.c           |    9 +++++++++
>>  usr/tgtd.h             |    5 +++++
>>  usr/util.c             |    8 +++++---
>>  usr/util.h             |    3 ++-
>>  9 files changed, 49 insertions(+), 15 deletions(-)
>>
>> diff --git a/doc/targets.conf.5.xml b/doc/targets.conf.5.xml
>> index f754df3..833e40a 100644
>> --- a/doc/targets.conf.5.xml
>> +++ b/doc/targets.conf.5.xml
>> @@ -336,7 +336,12 @@
>>       <listitem>
>>         <para>
>>           Specify the Logical blocks per physical block
>> -         exponent. This is an internal option that should not be
>> +         exponent. By default TGTD will set the lbppbe to automatically
>> +         match the underlying filesystem. Use this parameter to override
>> +         that setting.
>> +       </para>
>> +       <para>
>> +         This is an internal option that should not be
>>           set directly.
>>         </para>
>>       </listitem>
>> diff --git a/usr/bs_aio.c b/usr/bs_aio.c
>> index cbd91ba..75ae66f 100644
>> --- a/usr/bs_aio.c
>> +++ b/usr/bs_aio.c
>> @@ -304,6 +304,7 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>>  {
>>       struct bs_aio_info *info = BS_AIO_I(lu);
>>       int ret, afd;
>> +     uint32_t blksize = 0;
>>
>>       info->iodepth = AIO_MAX_IODEPTH;
>>       eprintf("create aio context for tgt:%d lun:%"PRId64 ", max iodepth:%d\n",
>> @@ -331,13 +332,14 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>>
>>       eprintf("open %s, RW, O_DIRECT for tgt:%d lun:%"PRId64 "\n",
>>               path, info->lu->tgt->tid, info->lu->lun);
>> -     *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|O_DIRECT, size);
>> +     *fd = backed_file_open(path, O_RDWR|O_LARGEFILE|O_DIRECT, size,
>> +                             &blksize);
>>       /* If we get access denied, try opening the file in readonly mode */
>>       if (*fd == -1 && (errno == EACCES || errno == EROFS)) {
>>               eprintf("open %s, READONLY, O_DIRECT for tgt:%d lun:%"PRId64 "\n",
>>                       path, info->lu->tgt->tid, info->lu->lun);
>>               *fd = backed_file_open(path, O_RDONLY|O_LARGEFILE|O_DIRECT,
>> -                                    size);
>> +                                    size, &blksize);
>>               lu->attrs.readonly = 1;
>>       }
>>       if (*fd < 0) {
>> @@ -346,11 +348,14 @@ static int bs_aio_open(struct scsi_lu *lu, char *path, int *fd, uint64_t *size)
>>               ret = *fd;
>>               goto remove_tgt_evt;
>>       }
>> -     else {
>> -             eprintf("%s opened successfully for tgt:%d lun:%"PRId64 "\n",
>> -                     path, info->lu->tgt->tid, info->lu->lun);
>> -             return 0;
>> -     }
>> +
>> +     eprintf("%s opened successfully for tgt:%d lun:%"PRId64 "\n",
>> +             path, info->lu->tgt->tid, info->lu->lun);
>> +
>> +     if (!lu->attrs.no_auto_lbppbe)
>> +             update_lbppbe(lu, blksize);
>
> Why can't we simply do:
>
>> +     if (!lu->lbppbe)
>> +             update_lbppbe(lu, blksize);
>
> ?
>
> If an user doesn't specify lbppbe via tgtadm, lu->lbppbe should be
> zero?
>

It can still be non-zero.

It is only zero first time we mount the backing file.

Since we support removable devices, a user could make a LUN/disk "offline"
which means we close the backing file.
Then a little while later the user swaps the media and makes the file
online again and we open a new file as the backing-store.


I.e.
1, create lun   and open backing file /fs1/foo.img    (lbppbe is
update automatically)
2, user sets lbppbe manually via tgtadm

3, user makes the file offline and close() the backing file :
tgtadm --tid 1 --lun 1 --op update --mode logicalunit --name online --value No

4, user connects the LUN to a different backing file and makes it online again :
tgtadm --tid 1 --lun 1 --op update --mode logicalunit --name path
--value /fs2/bar.img

At this stage we open() a new backing store file  and lbppbe is no
longer zero. It is onlt zero for the first time we open a backing
store file for a specific LUN.




At this stage, the LUN already exist but we just open a new backing store file.
At this stage   lbppbe is not zero when we open the file, so we can
not from lbppbe itself
determine
"is lbppbe non-zero because it was set for the previous backing file
we used and we swapped to a different one
or is lbppbe non-zero because a user forced a value using tgtadm?"


regards
ronnie sahlberg
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html