Hi!
See my comments inline, marked [AndersW].
regards,
Anders Widell
On 04/04/2016 12:50 PM, A V Mahesh wrote:
> Hi Anders Widell ,
>
> *Thank for the review , please find my responses .*
>
> On 4/4/2016 3:51 PM, Anders Widell wrote:
>> Hi!
>>
>> Here are my comments:
>>
>> 1) I think you need to make this feature configurable, as I see it as
>> potentially not backwards compatible.
> *[AVM]* The APIs used to fail with TIMEOUT ERR continuously (CPND
> used to crash in memcopy) in case of low disk space then requested ,
> now this patch only makes the API succeed or returns
> correct error SA_AIS_ERR_NO_RESOURCES ,
> so in this scenario,in that perspective it is only an
> improvement to the behavior.
>
> And since this is a POSIX call, I think it should not
> cause any backward compatibility issue from library perspective.
[AndersW] The backwards compatibility problem is that you are
pre-allocating shared memory, which was not done earlier. This means
that the memory consumption of OpenSAF could become higher than before.
>>
>> 2) A question: Have you considered alternative solutions? For
>> example, installing a signal handler for SIGBUS to catch the problem
>> and then propagate the error back to the CKPT API user?
>
> *[AVM]* I did think about that. With this approach, the issue(of lack
> of memory) is prevented upfront by returning error at ckptopen()
> itself which is beneficial to the user.
>
> Please let me know why it would be backward incompatible,
> because this approach improves the application behavior.
[AndersW] This approach would be backwards compatible, since we are not
increasing the memory consumption.
>
> -AVM
>>
>> regards,
>> Anders Widell
>>
>> On 04/04/2016 05:57 AM, A V Mahesh wrote:
>>> Hi Anders Widell/Ramesh,
>>>
>>> Can you please review this ticket by today , this has to go under
>>> 5.0.FC tag.
>>>
>>> -AVM
>>>
>>>> osaf/libs/core/include/ncs_osprm.h | 1 +
>>>> osaf/libs/core/leap/os_defs.c | 13 ++++++++++---
>>>> osaf/services/saf/cpsv/cpnd/cpnd_proc.c | 1 +
>>>> osaf/services/saf/cpsv/cpnd/cpnd_res.c | 2 ++
>>>> osaf/services/saf/glsv/glnd/glnd_shm.c | 1 +
>>>> osaf/services/saf/mqsv/mqnd/mqnd_shm.c | 1 +
>>>> 6 files changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>>
>>>> Provided ensured disk space is allocated for
>>>> NCS_OS_POSIX_SHM_REQ_OPEN request using posix_fallocate()
>>>> so that application such as CPSV subsequent writes to bytes in the
>>>> specified range are guaranteed not to fail because of lack of disk
>>>> space.
>>>> Updated the Opensaf services according to new options based on
>>>> requirements .
>>>>
>>>> diff --git a/osaf/libs/core/include/ncs_osprm.h
>>>> b/osaf/libs/core/include/ncs_osprm.h
>>>> --- a/osaf/libs/core/include/ncs_osprm.h
>>>> +++ b/osaf/libs/core/include/ncs_osprm.h
>>>> @@ -564,6 +564,7 @@ uint32_t ncs_os_posix_mq(NCS_OS_POSIX_MQ
>>>> uint32_t i_flags;
>>>> uint32_t i_map_flags;
>>>> uint64_t i_size;
>>>> + bool ensures_space;
>>>> uint64_t i_offset;
>>>> void *o_addr;
>>>> int32_t o_fd;
>>>> diff --git a/osaf/libs/core/leap/os_defs.c
>>>> b/osaf/libs/core/leap/os_defs.c
>>>> --- a/osaf/libs/core/leap/os_defs.c
>>>> +++ b/osaf/libs/core/leap/os_defs.c
>>>> @@ -795,9 +795,16 @@ uint32_t ncs_os_posix_shm(NCS_OS_POSIX_S
>>>> if (req->info.open.o_fd < 0) {
>>>> return NCSCC_RC_FAILURE;
>>>> } else {
>>>> - if (ftruncate(req->info.open.o_fd, (off_t) shm_size /*
>>>> off_t == long */ ) < 0) {
>>>> - printf("ftruncate failed with errno value %d \n",
>>>> errno);
>>>> - return NCSCC_RC_FAILURE;
>>>> + if (req->info.open.ensures_space == true) {
>>>> + if (posix_fallocate(req->info.open.o_fd, 0,
>>>> shm_size) == ENOSPC) {
>>>> + LOG_ER("posix_shm:posix_fallocate failed()
>>>> with not enough space left ENOSPC: %d \n", errno);
>>>> + return NCSCC_RC_FAILURE;
>>>> + }
>>>> + } else {
>>>> + if (ftruncate(req->info.open.o_fd, (off_t)
>>>> shm_size /* off_t == long */ ) < 0) {
>>>> + printf("ftruncate failed with errno value %d
>>>> \n", errno);
>>>> + return NCSCC_RC_FAILURE;
>>>> + }
>>>> }
>>>> prot_flag =
>>>> ncs_shm_prot_flags(req->info.open.i_flags);
>>>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>>>> b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>>>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>>>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_proc.c
>>>> @@ -481,6 +481,7 @@ uint32_t cpnd_ckpt_replica_create(CPND_C
>>>> cp_node->replica_info.open.info.open.i_size =
>>>> sizeof(CPSV_CKPT_HDR) +
>>>> cp_node->create_attrib.maxSections * (sizeof(CPSV_SECT_HDR) +
>>>> cp_node->create_attrib.maxSectionSize);
>>>> + cp_node->replica_info.open.info.open.ensures_space = true;
>>>> cp_node->replica_info.open.info.open.i_offset = 0;
>>>> cp_node->replica_info.open.info.open.i_name = buf;
>>>> cp_node->replica_info.open.info.open.i_map_flags = MAP_SHARED;
>>>> diff --git a/osaf/services/saf/cpsv/cpnd/cpnd_res.c
>>>> b/osaf/services/saf/cpsv/cpnd/cpnd_res.c
>>>> --- a/osaf/services/saf/cpsv/cpnd/cpnd_res.c
>>>> +++ b/osaf/services/saf/cpsv/cpnd/cpnd_res.c
>>>> @@ -160,6 +160,7 @@ uint32_t cpnd_ckpt_replica_create_res(NC
>>>> open_req->type = NCS_OS_POSIX_SHM_REQ_OPEN;
>>>> open_req->info.open.i_size =
>>>> sizeof(CPSV_CKPT_HDR) + (cp_info->maxSections *
>>>> ((sizeof(CPSV_SECT_HDR) + cp_info->maxSecSize)));
>>>> + open_req->info.open.ensures_space = true;
>>>> open_req->info.open.i_offset = 0;
>>>> open_req->info.open.i_name = buf;
>>>> open_req->info.open.i_map_flags = MAP_SHARED;
>>>> @@ -344,6 +345,7 @@ void *cpnd_restart_shm_create(NCS_OS_POS
>>>> cpnd_open_req->info.open.i_size =
>>>> sizeof(CLIENT_HDR) + (MAX_CLIENTS * sizeof(CLIENT_INFO))
>>>> + sizeof(CKPT_HDR) +
>>>> (MAX_CKPTS * sizeof(CKPT_INFO));
>>>> + cpnd_open_req->info.open.ensures_space = true;
>>>> cpnd_open_req->info.open.i_offset = 0;
>>>> cpnd_open_req->info.open.i_name = buffer;
>>>> cpnd_open_req->info.open.i_map_flags = MAP_SHARED;
>>>> diff --git a/osaf/services/saf/glsv/glnd/glnd_shm.c
>>>> b/osaf/services/saf/glsv/glnd/glnd_shm.c
>>>> --- a/osaf/services/saf/glsv/glnd/glnd_shm.c
>>>> +++ b/osaf/services/saf/glsv/glnd/glnd_shm.c
>>>> @@ -71,6 +71,7 @@ static uint32_t glnd_shm_open(GLND_CB *c
>>>> glnd_open_req.type = NCS_OS_POSIX_SHM_REQ_OPEN;
>>>> glnd_open_req.info.open.i_size = shm_size;
>>>> + glnd_open_req.info.open.ensures_space = false;
>>>> glnd_open_req.info.open.i_offset = 0;
>>>> glnd_open_req.info.open.i_name = shm_name;
>>>> glnd_open_req.info.open.i_map_flags = MAP_SHARED;
>>>> diff --git a/osaf/services/saf/mqsv/mqnd/mqnd_shm.c
>>>> b/osaf/services/saf/mqsv/mqnd/mqnd_shm.c
>>>> --- a/osaf/services/saf/mqsv/mqnd/mqnd_shm.c
>>>> +++ b/osaf/services/saf/mqsv/mqnd/mqnd_shm.c
>>>> @@ -66,6 +66,7 @@ uint32_t mqnd_shm_create(MQND_CB *cb)
>>>> mqnd_open_req.type = NCS_OS_POSIX_SHM_REQ_OPEN;
>>>> mqnd_open_req.info.open.i_size =
>>>> sizeof(MQND_SHM_VERSION) + (sizeof(MQND_QUEUE_CKPT_INFO)
>>>> * cb->mqnd_shm.max_open_queues);
>>>> + mqnd_open_req.info.open.ensures_space = false;
>>>> mqnd_open_req.info.open.i_offset = 0;
>>>> mqnd_open_req.info.open.i_name = shm_name;
>>>> mqnd_open_req.info.open.i_map_flags = MAP_SHARED;
>>>>
>>>> ------------------------------------------------------------------------------
>>>>
>>>>
>>>> Transform Data into Opportunity.
>>>> Accelerate data analysis in your applications with
>>>> Intel Data Analytics Acceleration Library.
>>>> Click to learn more.
>>>> http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140
>>>> _______________________________________________
>>>> Opensaf-devel mailing list
>>>> [email protected]
>>>> https://lists.sourceforge.net/lists/listinfo/opensaf-devel
>>>
>>
>
------------------------------------------------------------------------------
_______________________________________________
Opensaf-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/opensaf-devel
