Parent pointer design bugs

Parent pointer design bugs

[Allison Henderson]

Hi all,

I've been working on a new xfstest for parent pointers, and discovered 
that parent pointers are not getting created for protofiles.  I'm
exploring the idea of adding the parent pointer in xfs_dir_createname 
(instead of xfs-create/xfs_link/xfs_rename).  But I think that means 
that I'd have to change xfs_dir_createname to take the new xfs_inode 
instead of just the xfs_ino_t.  Which I'm not sure is
going to work out in all cases where it is used.  Before I end up 
chasing it too long and changing the design too much, I wanted to get 
some feed back to see what folks thoughts were.  Thanks!

Allison Henderson

Re: Parent pointer design bugs

[Darrick J. Wong]

On Wed, Jan 10, 2018 at 12:46:16PM -0700, Allison Henderson wrote:
> Hi all,
> 
> I've been working on a new xfstest for parent pointers, and discovered
> that parent pointers are not getting created for protofiles.

xfsprogs, IOWs.

> I'm exploring the idea of adding the parent pointer in
> xfs_dir_createname (instead of xfs-create/xfs_link/xfs_rename).  But I
> think that means that I'd have to change xfs_dir_createname to take
> the new xfs_inode instead of just the xfs_ino_t.

Yes, you'd probably want to pass the inode to xfs_dir_createname (and I
suppose xfs_dir_removename) if you decide to move the point at which you
queue the pptr defer op into the _dir_ functions.

However, consider the repair case where we either (a) have a dir entry
and no pptr or (b) have a pptr but no dir entry.  It's easy to repair
the directory links in either scenario if directory updates and parent
pointer updates are kept as separate libxfs calls.

There's also mkfs/proto.c which want to be able to create an arbitrary
directory tree in exactly the same way as a regular creation would.  It
seems to me that the orphanage management functions in repair/phase6.c
are doing basically the same sort of things -- creating the lost+found
directory, linking orphaned inodes into said lost+found directory,
re-stuffing collected directory entries into a directory, etc.  Perhaps
it makes more sense to hoist some of the fs/xfs/xfs_inode.c functions
into libxfs, add the pptr calls to those functions, port that to
xfsprogs, and then refactor mkfs/repair to use the new libxfs functions?

FWIW xfs_inode.c is already ~3600 lines long and contains routines for
handling inodes and for various directory manipulations involving
inodes, so I think it's fine to break that file up.

> Which I'm not sure is going to work out in all cases where it is used.
> Before I end up chasing it too long and changing the design too much,
> I wanted to get some feed back to see what folks thoughts were.
> Thanks!

tldr: I'm leaning towards refactor and port some of the xfs_inode
routines to xfsprogs... :)

--D

> 
> Allison Henderson
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Parent pointer design bugs

[Allison Henderson]

On 01/10/2018 03:54 PM, Darrick J. Wong wrote:
> On Wed, Jan 10, 2018 at 12:46:16PM -0700, Allison Henderson wrote:
>> Hi all,
>>
>> I've been working on a new xfstest for parent pointers, and discovered
>> that parent pointers are not getting created for protofiles.
> 
> xfsprogs, IOWs.
> 
>> I'm exploring the idea of adding the parent pointer in
>> xfs_dir_createname (instead of xfs-create/xfs_link/xfs_rename).  But I
>> think that means that I'd have to change xfs_dir_createname to take
>> the new xfs_inode instead of just the xfs_ino_t.
> 
> Yes, you'd probably want to pass the inode to xfs_dir_createname (and I
> suppose xfs_dir_removename) if you decide to move the point at which you
> queue the pptr defer op into the _dir_ functions.
> 
> However, consider the repair case where we either (a) have a dir entry
> and no pptr or (b) have a pptr but no dir entry.  It's easy to repair
> the directory links in either scenario if directory updates and parent
> pointer updates are kept as separate libxfs calls.
> 
> There's also mkfs/proto.c which want to be able to create an arbitrary
> directory tree in exactly the same way as a regular creation would.  It
> seems to me that the orphanage management functions in repair/phase6.c
> are doing basically the same sort of things -- creating the lost+found
> directory, linking orphaned inodes into said lost+found directory,
> re-stuffing collected directory entries into a directory, etc.  Perhaps
> it makes more sense to hoist some of the fs/xfs/xfs_inode.c functions
> into libxfs, add the pptr calls to those functions, port that to
> xfsprogs, and then refactor mkfs/repair to use the new libxfs functions?
> 
> FWIW xfs_inode.c is already ~3600 lines long and contains routines for
> handling inodes and for various directory manipulations involving
> inodes, so I think it's fine to break that file up.
> 
>> Which I'm not sure is going to work out in all cases where it is used.
>> Before I end up chasing it too long and changing the design too much,
>> I wanted to get some feed back to see what folks thoughts were.
>> Thanks!
> 
> tldr: I'm leaning towards refactor and port some of the xfs_inode
> routines to xfsprogs... :)
> 

Alrighty, I'll see what I can work out.  Thank you!!

> --D
> 
>>
>> Allison Henderson
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=9lB1PDKzm2gfSE5y-lOY3Vh-lrKgdXAOkR4V96QBhB0&s=qvbtvoxhw2nHvyF_z-kqLeLED9hGVZb1cp_alEA5rNI&e=
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=9lB1PDKzm2gfSE5y-lOY3Vh-lrKgdXAOkR4V96QBhB0&s=qvbtvoxhw2nHvyF_z-kqLeLED9hGVZb1cp_alEA5rNI&e=
>