[PATCH] libfs/jffs2: Force the inode size to a non-zero value if a directory.

Sebastian Huber sebastian.huber at embedded-brains.de
Wed Mar 14 06:53:27 UTC 2018


On 13/03/18 22:45, Chris Johns wrote:
> On 13/03/2018 17:52, Sebastian Huber wrote:
>> On 13/03/18 07:50, Sebastian Huber wrote:
>>> On 13/03/18 07:47, Chris Johns wrote:
>>>> On 13/03/2018 17:18, Sebastian Huber wrote:
>>>>> On 13/03/18 00:49, Chris Johns wrote:
>>>>>> Set the inode size to 256 to work around a newlib scandir check where a
>>>>>> directory has to have a non-zero size to work. Set the size to greater than
>>>>>> 24 bytes, a small dirent size so the allocator in scandir works.
>>>>>>
>>>>>> The newlib scandir code should be updated to a more recent scandir from
>>>>>> FreeBSD where these checks have been removed. This change is a work
>>>>>> around to avoid new tools on the release branch.
>>>>>>
>>>>>> With this change scandir works on IMFS, RFS and JFFS2.
>>>>> I cannot find a scandir() test in the fstests for all file systems.
>>>>>
>>>>>> Closes #3332
>>>>>> ---
>>>>>>     cpukit/libfs/src/jffs2/src/fs-rtems.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> b/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> index ce84d44470..790929d856 100644
>>>>>> --- a/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> +++ b/cpukit/libfs/src/jffs2/src/fs-rtems.c
>>>>>> @@ -1512,6 +1512,9 @@ static int jffs2_read_inode (struct _inode *inode)
>>>>>>         inode->i_mtime = je32_to_cpu(latest_node.mtime);
>>>>>>         inode->i_ctime = je32_to_cpu(latest_node.ctime);
>>>>>>     +    if (S_ISDIR(inode->i_mode))
>>>>>> +        inode->i_size = 256;
>>>>>> +
>>>>>>         inode->i_nlink = f->inocache->pino_nlink;
>>>>>>         mutex_unlock(&f->sem);
>>>>> This code is from the original JFFS2 support for eCos. Which side-effects has
>>>>> this i_size change for directories? Why can't you place this hack in
>>>>> rtems_jffs2_fstat()? This would reduce the impact area of this change.
>>>>>
>>>> I did not know the history of the RTEMS wrapper. I just assumed all code in that
>>>> file is specific to RTEMS. The change I have makes things consistent. I still
>>>> think it is OK?
>>> What do you mean with consistent?
> The function being patched is internal to `fs-rtems.c` which is a wrapper for
> JFFS2. The consistent comment I made referred to any reference to the inode had
> the same value exported to RTEMS, ie consistent.
>
> The code may have come from ecos and if ecos is using newlib with scandir they
> have the same problem. I do not consider ecos as being upstream, rather this
> code has been taken 'as was' (?).
>
>> I thought this is a hack to make the Newlib scandir() happy?
> The issue is clearly in scandir. FreeBSD these days and glibc do not stat the
> directory for it's size, OpenSolaris still does. The proper fix is to scandir.
>
> Fixing scandir requires a newlib patch and that means an update to the RSB. If
> this is preferred please say, it however is a lot more work so I am reluctant to
> do this, I do not have the time.
>
> I have coded around the scandir issue with opendir and readdir and I suspect
> there are a number of cases of this happening. I have seen at least one other
> case and wondered at the time why opendir etc was being used instead of scandir.

Then we should open a new ticket for the scandir() problem in Newlib and 
reference this ticket in the workaround. Once Newlib is fixed we can 
remove the workaround.

>
>> This i_size is used in several places. How do you know that
>>> you didn't accidentally broke something?
> The only thing I can think of is reading a directory as a file and a 0 means do
> not read. That does not seem to be a problem with the patch, I can cat a
> directory and get the same sort of rubbish I get when doing this on any other
> host which implies the directory as a file does have a size. If I remove the
> patch the cat of the same directory gives the same result and that would imply
> something else is wrong or inconsistent so who knows if what we have there is
> correct.

If you read() from a directory, then you get a stream of struct dirent 
items, see rtems_jffs2_dir_read().

>
> The size of a directory is file system specific so I would say any code that
> assumes anything is file system specific and fragile and the scandir issue would
> seem to support this. I have built a large application which uses the JFFS2
> heavily for application installing and management with many files and I see no
> issue. I have no other test results to say otherwise.

Ok, I have difficulties to understand from the source code, why the 
i_size change has no impact on the internal file system operation:

grep -r '\<i_size\>' cpukit/libfs/
cpukit/libfs/src/jffs2/include/linux/kernel.h:#define 
truncate_setsize(x, y) do { (x)->i_size = (y); } while (0)
cpukit/libfs/src/jffs2/src/nodelist.c:/* Doesn't set inode->i_size */
cpukit/libfs/src/jffs2/src/dir-rtems.c: inode->i_size = datalen;
cpukit/libfs/src/jffs2/src/dir-rtems.c: ri->isize = ri->dsize = 
ri->csize = cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/dir-rtems.c: ri->totlen = 
cpu_to_je32(sizeof(*ri) + inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:            (unsigned 
int)inode->i_size, offset));
cpukit/libfs/src/jffs2/src/fs-rtems.c:  ri->isize = 
cpu_to_je32(max((uint32_t)inode->i_size, offset));
cpukit/libfs/src/jffs2/src/fs-rtems.c:  ri->offset = 
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:  ri->dsize = cpu_to_je32(offset - 
inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:  inode->i_size = offset;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  ri->isize = cpu_to_je32((ivalid 
& ATTR_SIZE)?iattr->ia_size:inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (ivalid & ATTR_SIZE && 
inode->i_size < iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c:          ri->dsize = 
cpu_to_je32(iattr->ia_size - inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:          ri->offset = 
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (ivalid & ATTR_SIZE && 
inode->i_size > iattr->ia_size)
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (ivalid & ATTR_SIZE && 
inode->i_size < iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c:          inode->i_size = 
iattr->ia_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c:     We are protected from a 
simultaneous write() extending i_size
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (ivalid & ATTR_SIZE && 
inode->i_size > iattr->ia_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c:  buf->st_size = inode->i_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (pos >= inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c:          uint32_t max_available = 
inode->i_size - pos_32;
cpukit/libfs/src/jffs2/src/fs-rtems.c:          pos = inode->i_size;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  if (pos > inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c:          ri.isize = 
cpu_to_je32(inode->i_size);
cpukit/libfs/src/jffs2/src/fs-rtems.c:          if (pos > inode->i_size) {
cpukit/libfs/src/jffs2/src/fs-rtems.c: inode->i_size = pos;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  inode->i_size = 0;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  inode->i_size = 0;
cpukit/libfs/src/jffs2/src/fs-rtems.c:  inode->i_size = 
je32_to_cpu(latest_node.isize);
cpukit/libfs/src/jffs2/src/os-rtems.h:#define JFFS2_F_I_SIZE(f) 
(OFNI_EDONI_2SFFJ(f)->i_size)
cpukit/libfs/src/jffs2/src/os-rtems.h:          off_t i_size; // For 
files only
cpukit/libfs/src/jffs2/src/gc.c:                 * from i_size since 
i_size may have not been updated yet */
cpukit/libfs/src/jffs2/src/gc.c:                 * from i_size since 
i_size may have not been updated yet */

In case

diff --git a/cpukit/libfs/src/jffs2/src/fs-rtems.c 
b/cpukit/libfs/src/jffs2/src/fs-rtems.c
index 652a8e7dbe..074c9640ab 100644
--- a/cpukit/libfs/src/jffs2/src/fs-rtems.c
+++ b/cpukit/libfs/src/jffs2/src/fs-rtems.c
@@ -411,7 +411,14 @@ static int rtems_jffs2_fstat(
         buf->st_nlink = inode->i_nlink;
         buf->st_uid = inode->i_uid;
         buf->st_gid = inode->i_gid;
-       buf->st_size = inode->i_size;
+
+       if (S_ISDIR(inode->i_mode)) {
+               /* Magic value to make Newlib scandir() happy */
+               buf->st_size = 256;
+       } else {
+               buf->st_size = inode->i_size;
+       }
+
         buf->st_atime = inode->i_atime;
         buf->st_mtime = inode->i_mtime;
         buf->st_ctime = inode->i_ctime;

is sufficient to fix the scandir() problem I would rather fix it like this.

>
>> We have also this comment:
>>
>> struct _inode {
>>      cyg_uint32        i_ino;
>>
>>      int            i_count;
>>      mode_t            i_mode;
>>      nlink_t            i_nlink; // Could we dispense with this?
>>      uid_t            i_uid;
>>      gid_t            i_gid;
>>      time_t            i_atime;
>>      time_t            i_mtime;
>>      time_t            i_ctime;
>> //    union {
>>          unsigned short    i_rdev; // For devices only
>>          struct _inode *    i_parent; // For directories only
>>          off_t        i_size; // For files only
> Is that a requirement or an observation?
>
> It could be related to the implementation and there not being a need to set this
> field for a directory so it never was, ie non-data nodes ...
>
> https://git.rtems.org/rtems/tree/cpukit/libfs/src/jffs2/src/readinode.c#n1207

With your change the comment should be updated/removed.

>
>> //    };
>>      struct super_block *    i_sb;
>>      struct jffs2_full_dirent * i_fd;
>>
>>      struct jffs2_inode_info    jffs2_i;
>>
>>          struct _inode *        i_cache_prev; // We need doubly-linked?
>>          struct _inode *        i_cache_next;
>> };
>>
>> Which is no longer valid with this change.
>>
> I do not think so looking at the code and the comment provides no clue.
>
> Chris

-- 
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax     : +49 89 189 47 41-09
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.




More information about the devel mailing list