bdbuf: Disk I/O buffering

[Eugeny S. Mints]

forgot to mention:

first, sorry for the patch format, second the patch is agains 4.6.0 and 
the third I've never compiled/tested it.

	Eugeny

Eugeny S. Mints wrote:
> Hello Kevin,
> 
> Kirspel, Kevin {Engineering - Osmetech CCD} wrote:
> 
>> I have built an rtems 4.6.0 release and it has worked beautifully for 2
>> years.  I added compact flash support to my build by using the IDE
>> interface in rtems.  Every thing works great as long as I keep the
>> compact flash card in the system. I wanted to add support to rtems so
>> that I may remove and insert the compact flash card whenever I want. I
>> now probe() the compact flash device to make sure it is in place before
>> I perform any ata_ioctl() commands. If the device is not in place, I
>> return -1 with errno = ENODEV. This causes the bdbuf_swapout_task()
>> function to enter an indefinite loop because it keeps on trying to call
>> the ata_ioctl() routine with no success.
>>
>> The following code is taken from the bdbuf_swapout_task() function in
>> bdbuf.c:
>>
>> /* transfer_sema initialized when bd_buf inserted in the mod chain
>>    first time */
>> result = dd->ioctl(dd->phys_dev->dev, BLKIO_REQUEST, &req);
>>
>> rtems_disk_release(dd);
>>
>> if (result == -1)
>> {
>>   bd_buf->status = RTEMS_IO_ERROR;
>>   bd_buf->error = errno;
>>   /* Release tasks waiting on syncing this buffer */
>>   _CORE_mutex_Flush(&bd_buf->transfer_sema, NULL,
>> CORE_MUTEX_STATUS_SUCCESSFUL);
>> }
>> else
>> {
>>   if (bd_buf->in_progress)
>>   {
>>     rtems_interrupt_disable(level);
>>     _CORE_mutex_Seize(&bd_buf->transfer_sema, 0, TRUE, 0, level);
>>   }
>> }
>>
>> I am not a disk buffering guru, but what can I do to the
>> bdbuf_swapout_task() ( or other functions ) to make it process the error
>> so that at the higher levels and error code is returned.
> 
> Apart from that there is a bug that you discovered, the higher levels 
> are aware about status/error of this low level I/O via bd_buf->status 
> and bd_buf->error fields which are filled either in "if (result == -1)" 
> branch above (the swapout task algo is: a higher level task is awaiting 
> for a buffer blocking on transfer sema; the buffer is removed from 
> swapout task's queue of buffers to process at Chain_Get() and at 
> transfer sema release time higher level task gets control on a buffer 
> back with bd_buf status and error fields set appropriately) or at 
> bdbuf_write_transfer_done() execution time.
> 
> The bug you discovered is the following:
> at the time ioclt returns "-1 " bd_buf->modified field is set. But a bit 
> below at
> 
>         if (bd_buf->use_count == 0)
>         {
>             if (bd_buf->modified)
>             {
>                 Chain_Append(&bd_ctx.mod, &bd_buf->link);
>                 rc = rtems_semaphore_release(bd_ctx.flush_sema);
>             }
>             else
>             {
>                 Chain_Append(&bd_pool->lru, &bd_buf->link);
>                 rc = rtems_semaphore_release(bd_pool->bufget_sema);
>             }
>         }
> 
> since bd_buf->modified is not re-set (at normal path this field is reset 
> at bdbuf_write_transfer_done() before this part of code execution) the 
> rtems_semaphore_release(bd_ctx.flush_sema) leads to the infinit cycling 
> in swapout task.
> 
> Please let us know whether the patch attached fixes your problem (at 
> least inifinit cycling).
> 
>>
>> My current scenario is this:
>>
>> 1. I boot the system with the compact flash card in place.
>> 2. After task initialization, my main task initializes the ide
>> partitions, mounts the drive, opens a file, writes data to the file,
>> closes the file, and unmounts the drive. This works great.
>> 3. I then remove the compact flash and repeat the process.
>> 4. During the mount process, the system hangs in bdbuf_swapout_task() do
>> to the fact that the dd->ioctl() returns -1;
> 
> Based on your scenario I'm concerned about whether the bug in swapout 
> taks is the main issue.
> My understanding is that the scenario with removed flash shoud end up in 
> mount() call (more exaclty in 
> msdos_initialize_support()->fat_init_volume_info()->rtems_disk_lookup() 
> (if you use fat fs ) since rtems_disk_lookup() should fail if there is 
> no actual disk) and has not to have any deal with swapout task at all.
> 
>     Eugeny
> 
>> Kevin Kirspel
>> Osmetech 235 Hembree Park Drive
>> Roswell GA, 30076
>> 770-510-4444 x568
>>
>>
>>
>>
> 
> 
> ------------------------------------------------------------------------
> 
> --- bdbuf.old.c	2005-07-04 15:21:52.000000000 +0400
> +++ bdbuf.c	2005-07-04 15:23:28.000000000 +0400
> @@ -1545,7 +1545,7 @@
>           * and release semaphore */
>          if (bd_buf->use_count == 0)
>          {
> -            if (bd_buf->modified)
> +            if ((bd_buf->status != RTEMS_IO_ERROR) && (bd_buf->modified))
>              {
>                  Chain_Append(&bd_ctx.mod, &bd_buf->link);
>                  rc = rtems_semaphore_release(bd_ctx.flush_sema);