[SPDK] Rocksdb test case issue

[SPDK] Rocksdb test case issue

[Sablok, Kunal]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

Hi,
I am facing one issue in Rocksdb test case. This is regarding https://ci.spdk.io/spdk/builds/review/28da853c42bd6979540e1824cc6ad0550021d7e2.1527713791/fedora-04/build.log
Analyzed the issue and found the problem, please find below details, we can discuss how to fix.


*        Module init and exit functions have a requirement to be asynchronous in nature where module init needs to initialize some data structures per core, so it uses spdk_for_each_thread() call for this purpose.

*        In module init(), spdk_for_each_thread() is used and it executes one function which initializes some data structures on that core. When init on all cores is done, callback is called, then it calls spdk_bdev_module_init_done() to notify module init is done. Similar things are there in module exit function, where it uses spdk_for_each_thread() to deallocate some data structures per core and when all cores are done processing later callback function calls spdk_bdev_module_finish_done(). This is what spdk_for_each_thread() behavior.

*        However above behavior is causing issues with RocksDb/BlobFs test case only (other test cases and other testing is working fine) where test case is hung.

*        Analyzed further why only RocksDb test case is hung, please find below details:

o   RocksDb test case has a behavior that it allocates new SPDK threads after all modules are initialized.

o   Since when my module was initializing at that time the number of threads were X, so on X cores only data structures were initialized.

o   Later RocksDB allocates more threads with name "spdk_rocksdb" which adds to g_threads data structure where data structures were not initialized.

o   Later when module exit was called, spdk_for_each_thread() tried to execute provided function on unwanted threads (which were rocksdb created threads where init was not at all happened). So it stuck there and never returns which cause RocksDB test case to hung forever.

Please let me know if issue and cause description are not clear.

Question is can't we allocate all RocksDB related threads before normal module_inits so that if module inits are using spdk_for_each_thread() then it should work fine in this case?

Regards,
Kunal

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 9018 bytes --]

Re: [SPDK] Rocksdb test case issue

[Sablok, Kunal]

[-- Attachment #1: Type: text/plain, Size: 2419 bytes --]

Hi,
Please comment on this.

Regards,
Kunal

From: Sablok, Kunal
Sent: Friday, June 1, 2018 12:55 PM
To: spdk(a)lists.01.org
Subject: Rocksdb test case issue

Hi,
I am facing one issue in Rocksdb test case. This is regarding https://ci.spdk.io/spdk/builds/review/28da853c42bd6979540e1824cc6ad0550021d7e2.1527713791/fedora-04/build.log
Analyzed the issue and found the problem, please find below details, we can discuss how to fix.


*        Module init and exit functions have a requirement to be asynchronous in nature where module init needs to initialize some data structures per core, so it uses spdk_for_each_thread() call for this purpose.

*        In module init(), spdk_for_each_thread() is used and it executes one function which initializes some data structures on that core. When init on all cores is done, callback is called, then it calls spdk_bdev_module_init_done() to notify module init is done. Similar things are there in module exit function, where it uses spdk_for_each_thread() to deallocate some data structures per core and when all cores are done processing later callback function calls spdk_bdev_module_finish_done(). This is what spdk_for_each_thread() behavior.

*        However above behavior is causing issues with RocksDb/BlobFs test case only (other test cases and other testing is working fine) where test case is hung.

*        Analyzed further why only RocksDb test case is hung, please find below details:

o   RocksDb test case has a behavior that it allocates new SPDK threads after all modules are initialized.

o   Since when my module was initializing at that time the number of threads were X, so on X cores only data structures were initialized.

o   Later RocksDB allocates more threads with name "spdk_rocksdb" which adds to g_threads data structure where data structures were not initialized.

o   Later when module exit was called, spdk_for_each_thread() tried to execute provided function on unwanted threads (which were rocksdb created threads where init was not at all happened). So it stuck there and never returns which cause RocksDB test case to hung forever.

Please let me know if issue and cause description are not clear.

Question is can't we allocate all RocksDB related threads before normal module_inits so that if module inits are using spdk_for_each_thread() then it should work fine in this case?

Regards,
Kunal

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 9979 bytes --]

Re: [SPDK] Rocksdb test case issue

[Walker, Benjamin]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Tue, 2018-06-05 at 08:59 +0000, Sablok, Kunal wrote:
> Hi,
> 
> * Module init and exit functions have a requirement to be asynchronous in
> nature where module init needs to initialize some data structures per core, so
> it uses spdk_for_each_thread() call for this purpose.
> * In module init(), spdk_for_each_thread() is used and it executes one
> function which initializes some data structures on that core. When init on all
> cores is done, callback is called, then it calls spdk_bdev_module_init_done()
> to notify module init is done. Similar things are there in module exit
> function, where it uses spdk_for_each_thread() to deallocate some data
> structures per core and when all cores are done processing later callback
> function calls spdk_bdev_module_finish_done(). This is what
> spdk_for_each_thread() behavior.


The root of the problem here is that you have a global array, indexed by the
core number. Instead of storing data structures per core, you should be
leveraging SPDK's I/O channel functionality. Some background information is
available at http://www.spdk.io/doc/concurrency.html.

Specifically, inside pvol_bdev_create_cb you should be allocating a new
pvol_bdev_io_waitq data structure and stuffing it into the channel. Similarly in
pvol_bdev_destroy_cb you should be releasing it. Then, on start up and shutdown,
instead of using spdk_for_each_thread, call spdk_for_each_channel. With that
design, threads will be able to come and go as needed.

Thanks,
Ben

Re: [SPDK] Rocksdb test case issue

[Sablok, Kunal]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

Thanks a lot Ben.
Earlier I did it the same way you described i.e. in create_cb function but only issue I found was I required to create poller only for per core (or per channel) but create_cb was calling this for per core per io device. That way it was creating too many pollers per core per io device, so thought of creating per core only to limit it.
Is there any other better way where we can create pollers only per core, one I can think is create poller in create_cb function only but check if it is already created for this core don't create more, similarly in destroy_cb but here I need to track of if this is the last call of destroy_cb for this core?

Regards,
Kunal

-----Original Message-----
From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, Benjamin
Sent: Thursday, June 7, 2018 3:55 AM
To: spdk(a)lists.01.org
Subject: Re: [SPDK] Rocksdb test case issue

On Tue, 2018-06-05 at 08:59 +0000, Sablok, Kunal wrote:
> Hi,
> 
> * Module init and exit functions have a requirement to be asynchronous 
> in nature where module init needs to initialize some data structures 
> per core, so it uses spdk_for_each_thread() call for this purpose.
> * In module init(), spdk_for_each_thread() is used and it executes one 
> function which initializes some data structures on that core. When 
> init on all cores is done, callback is called, then it calls 
> spdk_bdev_module_init_done() to notify module init is done. Similar 
> things are there in module exit function, where it uses 
> spdk_for_each_thread() to deallocate some data structures per core and 
> when all cores are done processing later callback function calls 
> spdk_bdev_module_finish_done(). This is what
> spdk_for_each_thread() behavior.


The root of the problem here is that you have a global array, indexed by the core number. Instead of storing data structures per core, you should be leveraging SPDK's I/O channel functionality. Some background information is available at http://www.spdk.io/doc/concurrency.html.

Specifically, inside pvol_bdev_create_cb you should be allocating a new pvol_bdev_io_waitq data structure and stuffing it into the channel. Similarly in pvol_bdev_destroy_cb you should be releasing it. Then, on start up and shutdown, instead of using spdk_for_each_thread, call spdk_for_each_channel. With that design, threads will be able to come and go as needed.

Thanks,
Ben

_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] Rocksdb test case issue

[Walker, Benjamin]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

On Wed, 2018-06-06 at 23:16 +0000, Sablok, Kunal wrote:
> Thanks a lot Ben.
> Earlier I did it the same way you described i.e. in create_cb function but
> only issue I found was I required to create poller only for per core (or per
> channel) but create_cb was calling this for per core per io device. That way
> it was creating too many pollers per core per io device, so thought of
> creating per core only to limit it.
> Is there any other better way where we can create pollers only per core, one I
> can think is create poller in create_cb function only but check if it is
> already created for this core don't create more, similarly in destroy_cb but
> here I need to track of if this is the last call of destroy_cb for this core?

For each io_device registered, only one channel per thread will be constructed.
Multiple calls to get a channel for a given io_device on the same thread will
just return the existing channel. So if you register some global object as an
io_device, getting channels on that will yield just one per thread. You are
currently registering the bdevs as your io_device, so you'll get one channel per
bdev per thread.

> 
> Regards,
> Kunal
> 
> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, Benjamin
> Sent: Thursday, June 7, 2018 3:55 AM
> To: spdk(a)lists.01.org
> Subject: Re: [SPDK] Rocksdb test case issue
> 
> On Tue, 2018-06-05 at 08:59 +0000, Sablok, Kunal wrote:
> > Hi,
> > 
> > * Module init and exit functions have a requirement to be asynchronous 
> > in nature where module init needs to initialize some data structures 
> > per core, so it uses spdk_for_each_thread() call for this purpose.
> > * In module init(), spdk_for_each_thread() is used and it executes one 
> > function which initializes some data structures on that core. When 
> > init on all cores is done, callback is called, then it calls 
> > spdk_bdev_module_init_done() to notify module init is done. Similar 
> > things are there in module exit function, where it uses 
> > spdk_for_each_thread() to deallocate some data structures per core and 
> > when all cores are done processing later callback function calls 
> > spdk_bdev_module_finish_done(). This is what
> > spdk_for_each_thread() behavior.
> 
> 
> The root of the problem here is that you have a global array, indexed by the
> core number. Instead of storing data structures per core, you should be
> leveraging SPDK's I/O channel functionality. Some background information is
> available at http://www.spdk.io/doc/concurrency.html.
> 
> Specifically, inside pvol_bdev_create_cb you should be allocating a new
> pvol_bdev_io_waitq data structure and stuffing it into the channel. Similarly
> in pvol_bdev_destroy_cb you should be releasing it. Then, on start up and
> shutdown, instead of using spdk_for_each_thread, call spdk_for_each_channel.
> With that design, threads will be able to come and go as needed.
> 
> Thanks,
> Ben
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] Rocksdb test case issue

[Sablok, Kunal]

[-- Attachment #1: Type: text/plain, Size: 4027 bytes --]

Thanks Ben, As I understand from one of your recent review comment, somebody is working on SPDK API spdk_bdev_io_reserve(). I will be using this API and then waitq and poller will be removed, also the RocksDB test case issue should go away with this. Please let me know when spdk_bdev_io_reserve() is up in SPDK, I will merge with this.

Regards,
Kunal

-----Original Message-----
From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, Benjamin
Sent: Friday, June 8, 2018 10:12 PM
To: spdk(a)lists.01.org
Subject: Re: [SPDK] Rocksdb test case issue

On Wed, 2018-06-06 at 23:16 +0000, Sablok, Kunal wrote:
> Thanks a lot Ben.
> Earlier I did it the same way you described i.e. in create_cb function 
> but only issue I found was I required to create poller only for per 
> core (or per
> channel) but create_cb was calling this for per core per io device. 
> That way it was creating too many pollers per core per io device, so 
> thought of creating per core only to limit it.
> Is there any other better way where we can create pollers only per 
> core, one I can think is create poller in create_cb function only but 
> check if it is already created for this core don't create more, 
> similarly in destroy_cb but here I need to track of if this is the last call of destroy_cb for this core?

For each io_device registered, only one channel per thread will be constructed.
Multiple calls to get a channel for a given io_device on the same thread will just return the existing channel. So if you register some global object as an io_device, getting channels on that will yield just one per thread. You are currently registering the bdevs as your io_device, so you'll get one channel per bdev per thread.

> 
> Regards,
> Kunal
> 
> -----Original Message-----
> From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, 
> Benjamin
> Sent: Thursday, June 7, 2018 3:55 AM
> To: spdk(a)lists.01.org
> Subject: Re: [SPDK] Rocksdb test case issue
> 
> On Tue, 2018-06-05 at 08:59 +0000, Sablok, Kunal wrote:
> > Hi,
> > 
> > * Module init and exit functions have a requirement to be 
> > asynchronous in nature where module init needs to initialize some 
> > data structures per core, so it uses spdk_for_each_thread() call for this purpose.
> > * In module init(), spdk_for_each_thread() is used and it executes 
> > one function which initializes some data structures on that core. 
> > When init on all cores is done, callback is called, then it calls
> > spdk_bdev_module_init_done() to notify module init is done. Similar 
> > things are there in module exit function, where it uses
> > spdk_for_each_thread() to deallocate some data structures per core 
> > and when all cores are done processing later callback function calls 
> > spdk_bdev_module_finish_done(). This is what
> > spdk_for_each_thread() behavior.
> 
> 
> The root of the problem here is that you have a global array, indexed 
> by the core number. Instead of storing data structures per core, you 
> should be leveraging SPDK's I/O channel functionality. Some background 
> information is available at http://www.spdk.io/doc/concurrency.html.
> 
> Specifically, inside pvol_bdev_create_cb you should be allocating a 
> new pvol_bdev_io_waitq data structure and stuffing it into the 
> channel. Similarly in pvol_bdev_destroy_cb you should be releasing it. 
> Then, on start up and shutdown, instead of using spdk_for_each_thread, call spdk_for_each_channel.
> With that design, threads will be able to come and go as needed.
> 
> Thanks,
> Ben
> 
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
> _______________________________________________
> SPDK mailing list
> SPDK(a)lists.01.org
> https://lists.01.org/mailman/listinfo/spdk
_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk

Re: [SPDK] Rocksdb test case issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 4962 bytes --]

Hi Kunal,

I’m working on this spdk_bdev_io_reserve API.  I will keep you posted as I make progress.

Part of this effort will be adding the ability to configure the number of spdk_bdev_io structures in the global pool and the per-thread caches to help us stress these types of cases more extensively.

-Jim


On 6/11/18, 8:30 AM, "SPDK on behalf of Sablok, Kunal" <spdk-bounces(a)lists.01.org on behalf of kunal.sablok(a)intel.com> wrote:

    Thanks Ben, As I understand from one of your recent review comment, somebody is working on SPDK API spdk_bdev_io_reserve(). I will be using this API and then waitq and poller will be removed, also the RocksDB test case issue should go away with this. Please let me know when spdk_bdev_io_reserve() is up in SPDK, I will merge with this.
    
    Regards,
    Kunal
    
    -----Original Message-----
    From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, Benjamin
    Sent: Friday, June 8, 2018 10:12 PM
    To: spdk(a)lists.01.org
    Subject: Re: [SPDK] Rocksdb test case issue
    
    On Wed, 2018-06-06 at 23:16 +0000, Sablok, Kunal wrote:
    > Thanks a lot Ben.
    > Earlier I did it the same way you described i.e. in create_cb function 
    > but only issue I found was I required to create poller only for per 
    > core (or per
    > channel) but create_cb was calling this for per core per io device. 
    > That way it was creating too many pollers per core per io device, so 
    > thought of creating per core only to limit it.
    > Is there any other better way where we can create pollers only per 
    > core, one I can think is create poller in create_cb function only but 
    > check if it is already created for this core don't create more, 
    > similarly in destroy_cb but here I need to track of if this is the last call of destroy_cb for this core?
    
    For each io_device registered, only one channel per thread will be constructed.
    Multiple calls to get a channel for a given io_device on the same thread will just return the existing channel. So if you register some global object as an io_device, getting channels on that will yield just one per thread. You are currently registering the bdevs as your io_device, so you'll get one channel per bdev per thread.
    
    > 
    > Regards,
    > Kunal
    > 
    > -----Original Message-----
    > From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Walker, 
    > Benjamin
    > Sent: Thursday, June 7, 2018 3:55 AM
    > To: spdk(a)lists.01.org
    > Subject: Re: [SPDK] Rocksdb test case issue
    > 
    > On Tue, 2018-06-05 at 08:59 +0000, Sablok, Kunal wrote:
    > > Hi,
    > > 
    > > * Module init and exit functions have a requirement to be 
    > > asynchronous in nature where module init needs to initialize some 
    > > data structures per core, so it uses spdk_for_each_thread() call for this purpose.
    > > * In module init(), spdk_for_each_thread() is used and it executes 
    > > one function which initializes some data structures on that core. 
    > > When init on all cores is done, callback is called, then it calls
    > > spdk_bdev_module_init_done() to notify module init is done. Similar 
    > > things are there in module exit function, where it uses
    > > spdk_for_each_thread() to deallocate some data structures per core 
    > > and when all cores are done processing later callback function calls 
    > > spdk_bdev_module_finish_done(). This is what
    > > spdk_for_each_thread() behavior.
    > 
    > 
    > The root of the problem here is that you have a global array, indexed 
    > by the core number. Instead of storing data structures per core, you 
    > should be leveraging SPDK's I/O channel functionality. Some background 
    > information is available at http://www.spdk.io/doc/concurrency.html.
    > 
    > Specifically, inside pvol_bdev_create_cb you should be allocating a 
    > new pvol_bdev_io_waitq data structure and stuffing it into the 
    > channel. Similarly in pvol_bdev_destroy_cb you should be releasing it. 
    > Then, on start up and shutdown, instead of using spdk_for_each_thread, call spdk_for_each_channel.
    > With that design, threads will be able to come and go as needed.
    > 
    > Thanks,
    > Ben
    > 
    > _______________________________________________
    > SPDK mailing list
    > SPDK(a)lists.01.org
    > https://lists.01.org/mailman/listinfo/spdk
    > _______________________________________________
    > SPDK mailing list
    > SPDK(a)lists.01.org
    > https://lists.01.org/mailman/listinfo/spdk
    _______________________________________________
    SPDK mailing list
    SPDK(a)lists.01.org
    https://lists.01.org/mailman/listinfo/spdk
    _______________________________________________
    SPDK mailing list
    SPDK(a)lists.01.org
    https://lists.01.org/mailman/listinfo/spdk
    

Re: [SPDK] Rocksdb test case issue

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

Hi Kunal,

We have come up with a slightly different approach than spdk_bdev_io_reserve() – this one is called spdk_bdev_queue_io_wait().  Please see https://review.gerrithub.io/#/c/spdk/spdk/+/415074/.

When an SPDK bdev I/O submission function is called (i.e. read, readv, read_blocks, write, unmap, etc.) and it fails with –ENOMEM, it means no spdk_bdev_io structure was available.  The caller can then request a callback using spdk_bdev_queue_io_wait() to be notified when an spdk_bdev_io is available on the same thread.  The caller is responsible for providing the storage for the spdk_bdev_io_wait_entry structure, and saving any state so that it knows where to continue when the spdk_bdev_io is available.  In your striping case, that might mean keeping track if the first child I/O is successfully submitted, but you need to wait for an spdk_bdev_io for subsequent child I/O.  Even the most basic case, where the striping translation results in only one child I/O – and that one fails with –ENOMEM – will need to be accounted for.  (All virtual bdev modules will need to make similar changes).

Note that other patches have been pushed this week to allow specifying the spdk_bdev_io pool size - specifically to help with developing and testing this new code and applications/drivers that will be using the new API.

The patch still needs a bit more work but I wanted to bring it to your attention so you could provide feedback.  The bdevperf utility has been updated in this patch to make use of the new API and served well as a test case (i.e. limit the spdk_bdev_io pool to 32 but run with a queue depth of 128).

Regards,

-Jim



On 6/11/18, 8:30 AM, "SPDK on behalf of Sablok, Kunal" <spdk-bounces(a)lists.01.org on behalf of kunal.sablok(a)intel.com> wrote:

    Thanks Ben, As I understand from one of your recent review comment, somebody is working on SPDK API spdk_bdev_io_reserve(). I will be using this API and then waitq and poller will be removed, also the RocksDB test case issue should go away with this. Please let me know when spdk_bdev_io_reserve() is up in SPDK, I will merge with this.
    
    Regards,
    Kunal
    
[…]

Re: [SPDK] Rocksdb test case issue

[Sablok, Kunal]

[-- Attachment #1: Type: text/plain, Size: 2682 bytes --]

Thanks Jim for informing. I have posted some of the questions in the review itself.

Regards,
Kunal

-----Original Message-----
From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Friday, June 15, 2018 6:39 AM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] Rocksdb test case issue

Hi Kunal,

We have come up with a slightly different approach than spdk_bdev_io_reserve() – this one is called spdk_bdev_queue_io_wait().  Please see https://review.gerrithub.io/#/c/spdk/spdk/+/415074/.

When an SPDK bdev I/O submission function is called (i.e. read, readv, read_blocks, write, unmap, etc.) and it fails with –ENOMEM, it means no spdk_bdev_io structure was available.  The caller can then request a callback using spdk_bdev_queue_io_wait() to be notified when an spdk_bdev_io is available on the same thread.  The caller is responsible for providing the storage for the spdk_bdev_io_wait_entry structure, and saving any state so that it knows where to continue when the spdk_bdev_io is available.  In your striping case, that might mean keeping track if the first child I/O is successfully submitted, but you need to wait for an spdk_bdev_io for subsequent child I/O.  Even the most basic case, where the striping translation results in only one child I/O – and that one fails with –ENOMEM – will need to be accounted for.  (All virtual bdev modules will need to make similar changes).

Note that other patches have been pushed this week to allow specifying the spdk_bdev_io pool size - specifically to help with developing and testing this new code and applications/drivers that will be using the new API.

The patch still needs a bit more work but I wanted to bring it to your attention so you could provide feedback.  The bdevperf utility has been updated in this patch to make use of the new API and served well as a test case (i.e. limit the spdk_bdev_io pool to 32 but run with a queue depth of 128).

Regards,

-Jim



On 6/11/18, 8:30 AM, "SPDK on behalf of Sablok, Kunal" <spdk-bounces(a)lists.01.org on behalf of kunal.sablok(a)intel.com> wrote:

    Thanks Ben, As I understand from one of your recent review comment, somebody is working on SPDK API spdk_bdev_io_reserve(). I will be using this API and then waitq and poller will be removed, also the RocksDB test case issue should go away with this. Please let me know when spdk_bdev_io_reserve() is up in SPDK, I will merge with this.
    
    Regards,
    Kunal
    
[…]

_______________________________________________
SPDK mailing list
SPDK(a)lists.01.org
https://lists.01.org/mailman/listinfo/spdk