[PATCH 1/3] Filesystem: PR1398: Fix lseek() mechanic

[Sebastian Huber]

On 05/09/2012 01:53 AM, Chris Johns wrote:
> On 9/05/12 5:17 AM, Sebastian Huber wrote:
>> On 08/05/12 08:43, Chris Johns wrote:
>>> On 8/05/12 3:34 PM, Sebastian Huber wrote:
>>>>
>>>> Is the complex rtems_rfs_rtems_file_lseek() really necessary? What
>>>> happens if we simply change the iop->offset? Will the read and write
>>>> handler cope with that?
>>>>
>>>
>>> I do not know. The current file seek code plays with buffers. I am not
>>> sure what effect this may have if the code was removed. I can see
>>> moving this code would make the seek cheaper and if someone is seeking
>>> with no I/O there is a cost.
>>>
>>> Is there another issue I am missing ?
>>
>> I only wondered why the [PATCH 2/3] Filesystem: PR1871: Fix O_APPEND
>> works (mrfs_fsrdwr test case).
>>
>
> Ah ok. I have taken a look at your repo in more detail and I do not think the
> RFS change will work as is.
>
> I think order for this approach to work you need to perform an RFS file layer
> seek call (rather than the RFS RTEMS layer) if the iop->offset does not match
> the RFS handle's position.
>
> You could add a check in the RFS file layer seek call to see if the handle
> matches the new position and do nothing if it does. This should be light weight.
>
> The important path here is not seeking, it is the write and read path and the
> common case is repeated operations so for performance reasons it is assumed the
> buffering is set up and ready and these path only do any extra work when moving
> to a new block. If a seek happens it resets the buffering so the next read or
> write call will move to the correct file block.
>
> I am also not certain we should capture the O_APPEND open state in the RFS
> RTEMS layer. It should be in the RFS file layer. I was not aware of the
> requirement in append mode that all writes must append. I had assumed this only
> positioned the write pointer to the end of the file when opening. I understand
> if you do not wish to make this change how-ever could you please raise a PR to
> have the code moved.

Ok, I will try to fix this and send a new patch set.

In the rtems_rfs_rtems_file_write() I don't understand this code:

   pos = iop->offset;

   /*
    * If the iop position is past the physical end of the file we need to set
    * the file size to the new length before writing. If the position equals the
    * size of file we are still past the end of the file as positions number
    * from 0. For a specific position we need a file that has a length of one
    * more.
    */

   if (pos >= rtems_rfs_file_size (file))
   {
     rc = rtems_rfs_file_set_size (file, pos + 1);
     if (rc)
     {
       rtems_rfs_rtems_unlock (rtems_rfs_file_fs (file));
       return rtems_rfs_rtems_error ("file-write: write extend", rc);
     }
   }

   rtems_rfs_file_set_bpos (file, pos);

Why do we set the file size to "pos + 1" and not "pos + count"?  How is the 
EFBIG error case described in POSIX write() handled?

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.