What we could do is
When we open the backing store, we always set lbppbe based on the
blocksize of the backing file.
Then, if the user wants a different lbppbe, he/she can set it via tgtadm.
If the user then makes the lun offline/online this resets the lbppbe
back to the blocksize of the backing file
so the user is then responsible to use tgtadm to reset the lbppbe
value back to what he/she wants.
Then document the fact that "once you set lbppbe using tgtadm, this
only lasts until the LUN is made offline/online. lbppbe are reset
across a offline/online operation so if you really need a specific
value you have to reset it with tgtadm after every offline/online
cycle".
regards
ronnie sahlberg
On Wed, Apr 18, 2012 at 9:45 PM, ronnie sahlberg
<ronniesahlberg@xxxxxxxxx> wrote:
> On Wed, Apr 18, 2012 at 9:30 PM, FUJITA Tomonori
> <fujita.tomonori@xxxxxxxxxxxxx> wrote:
>> On Wed, 18 Apr 2012 19:26:47 +1000
>> Ronnie Sahlberg <ronniesahlberg@xxxxxxxxx> 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@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
