Bug #2122

[Review] Fixes to the VFS layer

Added by ftigeot over 2 years ago. Updated over 2 years ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Hi,

While working on the vfs-quota branch, I found some problems in the existing
kernel VFS code.

The attached patches correspond to local commits I have created to fix them.
I'm not too sure if what I've done is ideal, and I'd like these patches
to be reviewed before pushing them to master (or not).

They have been applied to two of my machines (one desktop, one database
server), and do not seem to cause any problem.

0008-Fix-a-bug-in-nullfs_mount.patch Magnifier (1.08 KB) ftigeot, 08/28/2011 10:26 AM

0023-Populate-the-statfs-structure-associated-to-mount-po.patch Magnifier (1.1 KB) ftigeot, 08/28/2011 10:26 AM

0032-Run-VFS_START-for-the-root-mount-point.patch Magnifier (1.75 KB) ftigeot, 08/28/2011 10:26 AM

0001-Fix-a-bug-in-nullfs_mount-and-nullfs_statfs.patch Magnifier (1.43 KB) ftigeot, 09/02/2011 08:01 AM

History

#1 Updated by alexh over 2 years ago

Francois,

as I said previously on IRC I disagree with this approach to statfs and
hence to your first two patches. You should be using the statfs entry
point and not inspect the statfs structure in the mount point.

For that, you need to simply fix the right thing, which would be the
VOP_STATFS entry of nullfs and make it populate the fields you need as
you expect. On IRC I outlined this in much more detail, so please take a
look at your logs for more info.

That the stat field in the mount point is not populated as you would
like it is *not* a bug and doesn't really need fixing, you should be
using the right approach instead, one that is using the API instead of
raw accesses.

Regarding the other patch, please make sure that you don't break the
initrd infrastructure in the process. Haven't looked in detail at your
patch, but it looks relatively dangerous in assuming where the root
mount is in the mount list.

Kind Regards,
Alex

On 28/08/11 11:22, Francois Tigeot wrote:
> Hi,
>
> While working on the vfs-quota branch, I found some problems in the existing
> kernel VFS code.
>
> The attached patches correspond to local commits I have created to fix them.
> I'm not too sure if what I've done is ideal, and I'd like these patches
> to be reviewed before pushing them to master (or not).
>
> They have been applied to two of my machines (one desktop, one database
> server), and do not seem to cause any problem.
>

#2 Updated by ftigeot over 2 years ago

Thanks for the remarks Alex,

Alex Hornung wrote:
>
> as I said previously on IRC I disagree with this approach to statfs and
> hence to your first two patches. You should be using the statfs entry
> point and not inspect the statfs structure in the mount point.
>
> For that, you need to simply fix the right thing, which would be the
> VOP_STATFS entry of nullfs and make it populate the fields you need as
> you expect.

To be honest, I had forgotten about this conversation; I'll try to
replace the direct struct mount accesses by the use of VOP_STATFS.

> That the stat field in the mount point is not populated as you would
> like it is *not* a bug and doesn't really need fixing, you should be
> using the right approach instead, one that is using the API instead of
> raw accesses.

When I tried to use the STATFS function API, it didn't work as expected
and caused kernel panics.
I now see that this function was not completely implemented for tmpfs.

What is the rationale for not using the statfs information present in
struct mount ? It was in a very convenient location...

> Regarding the other patch, please make sure that you don't break the
> initrd infrastructure in the process. Haven't looked in detail at your
> patch, but it looks relatively dangerous in assuming where the root
> mount is in the mount list.

This is the part where I'm the less sure of what I should be doing. The
code was copied verbatim from the start_init() function; I assumed it
was correct in the first place.
I didn't remove it from there; when I tried to I once again got problems
with an unitialized namecache layer, but this time later in the boot
process.

Best Regards,

>> While working on the vfs-quota branch, I found some problems in the existing
>> kernel VFS code.
>>
>> The attached patches correspond to local commits I have created to fix them.
>> I'm not too sure if what I've done is ideal, and I'd like these patches
>> to be reviewed before pushing them to master (or not).

#3 Updated by ftigeot over 2 years ago

Francois Tigeot wrote:
> Alex Hornung wrote:
>>
>> as I said previously on IRC I disagree with this approach to statfs and
>> hence to your first two patches. You should be using the statfs entry
>> point and not inspect the statfs structure in the mount point.
>>
>> For that, you need to simply fix the right thing, which would be the
>> VOP_STATFS entry of nullfs and make it populate the fields you need as
>> you expect.
>
> To be honest, I had forgotten about this conversation; I'll try to
> replace the direct struct mount accesses by the use of VOP_STATFS.

I'm focusing on the first patch for now, the one which initializes
mp->mnt_stat.f_mntonname from nullfs_mount()

>> That the stat field in the mount point is not populated as you would
>> like it is *not* a bug and doesn't really need fixing, you should be
>> using the right approach instead, one that is using the API instead of
>> raw accesses.

Guess where VOP_STATFS takes the values it returns ? From mp->mnt_stat :
http://opengrok.wolfpond.org/source/xref/sys/vfs/nullfs/null_vfsops.c#nullfs_statfs

nullfs_statfs is a very simple function; it only copies existing values.

The initial name of the mount point can only be known from the
nullfs_mount function; if this patch is not applied, VOP_STATFS will
never be able to return the correct values.

I'm afraid patch-0008 has to be applied.

#4 Updated by ftigeot over 2 years ago

On Thu, Sep 01, 2011 at 11:38:42PM +0200, Francois Tigeot wrote:
>
> The initial name of the mount point can only be known from the
> nullfs_mount function; if this patch is not applied, VOP_STATFS will
> never be able to return the correct values.

New patch attached.

Also available in: Atom PDF