Parent pointers

DISCUSSION Allison Henderson 7 participants 16 messages Jul 13, 2017 – Jul 18, 2017

Share & tools

Allison Henderson

author

2017-07-13 23:25 UTC #0 lore raw

Hi all,

I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it. I got in touch with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!

Allison Henderson

Carlos Maiolino

2017-07-14 08:50 UTC #1 lore raw

Hi,

Quoted 4 lines · Allison Henderson

Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.

Are you talking about parent pointers in the BTrees?

Quoted 10 lines

I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!

It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.

Cheers

Signature · 1 line

Carlos

Eric Sandeen

2017-07-14 14:04 UTC #2 lore raw

Quoted 10 lines · Carlos Maiolino

Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?

No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.

"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."

It has a long and ... difficult history.

-Eric

Quoted 17 lines

Quoted 10 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Cheers

Allison Henderson

author

2017-07-14 18:03 UTC #3 lore raw

Quoted 17 lines · Eric Sandeen

Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.

Quoted 7 lines

"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."
It has a long and ... difficult history.
-Eric

Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.

My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.

Allison

Quoted 23 lines

Quoted 12 lines
Quoted 11 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
Quoted 6 lines
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Cheers
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2017-07-14 18:46 UTC #4 lore raw

Quoted 22 lines · Allison Henderson

Quoted 21 lines · Eric Sandeen
Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.
"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."

Quoted 6 lines

Quoted 3 lines
It has a long and ... difficult history.
-Eric
Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.
My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.

If you're forward-porting that 17-patch set from Mark, I'd suggest first reading Dave's response to it - IIRC it amounted to a firm NAK. it also highlights the complexity of this undertaking, and may explain why nobody has gotten it done (yet) :)

-Eric

Quoted 29 lines

Allison
Quoted 23 lines
Quoted 18 lines
Quoted 11 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Cheers
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brian Foster

2017-07-14 19:07 UTC #5 lore raw

Quoted 25 lines · Eric Sandeen

Quoted 24 lines · Allison Henderson
Quoted 23 lines · Eric Sandeen
Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.
"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."
It has a long and ... difficult history.

Quoted 9 lines

Quoted 4 lines
Quoted 1 line
-Eric
Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.
My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.
If you're forward-porting that 17-patch set from Mark, I'd suggest first reading Dave's response to it - IIRC it amounted to a firm NAK. it also highlights the complexity of this undertaking, and may explain why nobody has gotten it done (yet) :)

The patches that came from me (to Allison) were last sent to me directly from Dave. I know he had some objections to the original design, but my understanding was that he had incorporated fixes for those issues in his modifications to Mark's original series (such as the xattr format and whatnot), but the series wasn't completely done yet in terms of supporting all possible directory operations. (It's probably a good idea to review that old thread anyways just to confirm, unless Dave catches this and is able to chime in one way or the other.)

I basically just forwarded ported Dave's patches to more recent kernels and added a couple minor fixes as I had combed through the existing code. I never really got to adding anything of substance before Allison volunteered to take over the series.

FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is. It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches. IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)

Brian

Quoted 35 lines

-Eric
Quoted 17 lines
Allison
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Quoted 12 lines
Quoted 7 lines
Quoted 2 lines
Cheers
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Sandeen

2017-07-14 19:14 UTC #6 lore raw

Quoted 31 lines · Brian Foster

Quoted 30 lines · Eric Sandeen
Quoted 28 lines · Allison Henderson
Quoted 17 lines · Eric Sandeen
Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.
Quoted 7 lines
"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."
It has a long and ... difficult history.
-Eric
Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.
My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.

Quoted 7 lines

Quoted 4 lines
If you're forward-porting that 17-patch set from Mark, I'd suggest first reading Dave's response to it - IIRC it amounted to a firm NAK. it also highlights the complexity of this undertaking, and may explain why nobody has gotten it done (yet) :)
The patches that came from me (to Allison) were last sent to me directly from Dave.

Oh, ok. I didn't know we were talking about off-list patches, sorry if I missed that. I thought Allison was looking at the 17-patch series on the list, and wanted to make sure this wouldn't be a false start. :)

Thanks,
-Eric

Darrick J. Wong

2017-07-14 22:47 UTC #7 lore raw

In reply to Brian Foster

Quoted 20 lines · Brian Foster

Quoted 19 lines · Eric Sandeen
Quoted 18 lines · Allison Henderson
Quoted 17 lines · Eric Sandeen
Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."
It has a long and ... difficult history.
-Eric

Woot, time to dive in. :)

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and

Well, a lot of attributes, in the case of multiply hardlinked files. One concern I have is that since a file can be hardlinked 2^32 times and path components have a maximum length of 255 bytes, we'll have to be careful about not overflowing di_anextents as a part of making a potentially huge attribute tree.

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.

This (the disk format) is probably the most important part to get settled. FWIW, I've recaptured three of the previous discussions of parent pointers -- Lachlan McIlroy's code dump in 2009[1], Mark Tinguely's 2013 attempt[2] and 2014 attempt[3] at discussing patches. For those of you following along at home, the gmane link mentioned in
[3] is the link at [1].

[1] http://oss.sgi.com/archives/xfs/2009-01/msg01068.html
[2] http://oss.sgi.com/archives/xfs/2013-04/msg00214.html
[3] https://www.spinics.net/lists/xfs/msg25175.html

The latest round of parent pointer patches is (I think) in the form of an off-list patchset that Brian (and now Allison) are cleaning up for reposting, which hopefully happens soon because I feel rather ill at ease discussing off-list code that lacks S-o-b's.

Anyway, the current proposal is to create a new xattr namespace (ATTR_PARENT) and to fill it with (parent_ino, parent_gen, dirent_offset) => (dirent name) attributes:

struct xfs_parent_name_rec {
	__be64	p_ino;
	__be32	p_gen;
	__be32	p_diroffset;
};

p_diroffset is really a directory datapointer, not a raw byte offset.

We also have an incore data structure of:

struct xfs_parent_name_irec {
	__uint64_t	p_ino;
	__uint32_t	p_gen;
	__uint32_t	p_diroffset;
	const char	*p_name;
	__uint32_t	p_namelen;
};

I think p_ino ought to be xfs_ino_t and not uint64_t, and p_diroffset ought to be xfs_dir2_dataptr_t instead of uint32_t.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.

<nod>

Quoted 6 lines

Quoted 5 lines
If you're forward-porting that 17-patch set from Mark, I'd suggest first reading Dave's response to it - IIRC it amounted to a firm NAK. it also highlights the complexity of this undertaking, and may explain why nobody has gotten it done (yet) :)

Quoted 6 lines

The patches that came from me (to Allison) were last sent to me directly from Dave. I know he had some objections to the original design, but my understanding was that he had incorporated fixes for those issues in his modifications to Mark's original series (such as the xattr format and
From what I can tell, he seems to have taken Mark's patches to pass dir

offsets back from the dirent manipulation functions and then started writing a simple implementation of stuffing the attrs into the attr tree.

Quoted 4 lines

whatnot), but the series wasn't completely done yet in terms of supporting all possible directory operations. (It's probably a good idea to review that old thread anyways just to confirm, unless Dave catches this and is able to chime in one way or the other.)

(Or jump in the fray out once he's back from sabbatical.)

Quoted 8 lines

I basically just forwarded ported Dave's patches to more recent kernels and added a couple minor fixes as I had combed through the existing code. I never really got to adding anything of substance before Allison volunteered to take over the series.
FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is.

Most of the users of xfs_trans_roll seem to be buried in the xattr code. For example, _attr_set can end up rolling a transaction after converting an attr fork from short format to leaf format before retrying the attr add operation, but we don't use log redo items (er, defer_ops items) to make sure log recovery will restart the attr add operation if we crash before the final _trans_commit after calling _attr_{leaf,node}_addname.

In the case of a regular xattr set operation this wouldn't have been a big deal because the fsetxattr call wouldn't have returned, so all the user could possibly see is an inode with a perhaps unnecessarily large xattr fork. Now that we need to set xattrs in the same transaction chain as a directory operation, it becomes very important that log recovery gain the ability to resume an xattr add operation no matter where the log stopped. Creating a directory can become this longwinded pile of updates:

- Allocate directory block, map into data fork, chain rmap update, add directory entry, chain pptr update;
- Add rmap entry for new directory block (more chaining to fix freelist), finish first rmap update;
- Allocate xattr block for short->leaf conversion, map into attr fork, chain rmap update; chain xattr add operation;
- Add rmap entry for new xattr block (more chaining to fix freelist), finish second rmap update;
- Add xattr entry to file, finish xattr add operation;
- Finish pptr update.

Basically, I think someone's going to have to go audit all the uses of xfs_trans_roll in the attr code to figure out which operations need redo items, and how to cram everything toegether into a single xfs_defer_ops, rather than sprinkling them around the attr code like we do now, because redo items cannot be deferred from one defer_ops to another.

Quoted 2 lines

It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.

That's also a problem; unless someone can get Mark to supply them, we probably have to get someone to rewrite them. At a bare minimum I think we explicitly have to pass back a xfs_dir2_dataptr_t, not a bare uint32_t.

Quoted 4 lines

IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)

Agreed.

Quoted 41 lines

Brian
Quoted 31 lines
-Eric
Quoted 17 lines
Allison
Quoted 16 lines
Quoted 16 lines
Quoted 11 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Quoted 12 lines
Quoted 7 lines
Quoted 2 lines
Cheers
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Tinguely, Mark

2017-07-15 16:36 UTC #8 lore raw

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Darrick J. Wong
Sent: Friday, July 14, 2017 5:47 PM
To: Brian Foster <[email protected]>
Cc: Eric Sandeen <[email protected]>; Allison Henderson <[email protected]>; [email protected]; [email protected]
Subject: Re: Parent pointers

Quoted 4 lines · Brian Foster

Quoted 3 lines · Eric Sandeen
Quoted 2 lines · Allison Henderson
Quoted 1 line · Eric Sandeen

Quoted 2 lines

It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.

Me says:
The community can do what they want. If you want to start with that code, permission for S-O-B: me (snickers at the pun)

Quoted 4 lines

IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)

No one is asking me but I will add, IMO (definitely disclaiming the storage group and HPE):
the PIP costs outweigh the benefits.
Adding the redundant filename for recovery and an occasional output of the path pushes the cost to be *totally unreasonable*.
If I had a say, I don't and won't, I would NAK any implementation that included it.
Implementing with extended attributes is the wrong, wrong, wrong choice. It was done for hysterical :) reasons and even that appeasement bit me in the asterisk.

Good luck.

--Mark Tinguely
not representing anyone but myself.

Brian Foster

2017-07-17 14:48 UTC #12 lore raw

In reply to Darrick J. Wong

Quoted 134 lines · Darrick J. Wong

Quoted 20 lines · Brian Foster
Quoted 19 lines · Eric Sandeen
Quoted 18 lines · Allison Henderson
Quoted 17 lines · Eric Sandeen
Quoted 10 lines · Carlos Maiolino
Hi,
Quoted 4 lines · Allison Henderson
Hi all,
I've been doing some digging on adding parent pointers to xfs and wanted to send a note to folks here to get peoples opinions on it.
Are you talking about parent pointers in the BTrees?
No, see [RFC 00/17] RFC parent inode pointers. for example, from long ago.
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
"Parent inode support allow XFS to quickly derive a file name and path from the mount point. This can aid in directory/path policies and can help relocate items during filesystem shrink."
It has a long and ... difficult history.
-Eric
Woot, time to dive in. :)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Right, so to expand on Eric's answer, it looks like Dave and Brian had been working on some improvements based on that set, but it's not quite finished yet. The idea is that we add an extended attribute to keep track of the parents inode and generation, and
Well, a lot of attributes, in the case of multiply hardlinked files. One concern I have is that since a file can be hardlinked 2^32 times and path components have a maximum length of 255 bytes, we'll have to be careful about not overflowing di_anextents as a part of making a potentially huge attribute tree.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
also the child entries offset, and filename. So in this solution the EA is name={parent inode #, parent inode generation, dirent offset}, value={dirent filename}.
This (the disk format) is probably the most important part to get settled. FWIW, I've recaptured three of the previous discussions of parent pointers -- Lachlan McIlroy's code dump in 2009[1], Mark Tinguely's 2013 attempt[2] and 2014 attempt[3] at discussing patches. For those of you following along at home, the gmane link mentioned in
[3] is the link at [1].
[1] http://oss.sgi.com/archives/xfs/2009-01/msg01068.html
[2] http://oss.sgi.com/archives/xfs/2013-04/msg00214.html
[3] https://www.spinics.net/lists/xfs/msg25175.html
The latest round of parent pointer patches is (I think) in the form of an off-list patchset that Brian (and now Allison) are cleaning up for reposting, which hopefully happens soon because I feel rather ill at ease discussing off-list code that lacks S-o-b's.
Anyway, the current proposal is to create a new xattr namespace (ATTR_PARENT) and to fill it with (parent_ino, parent_gen, dirent_offset) => (dirent name) attributes:
struct xfs_parent_name_rec {
	__be64	p_ino;
	__be32	p_gen;
	__be32	p_diroffset;
};
p_diroffset is really a directory datapointer, not a raw byte offset.
We also have an incore data structure of:
struct xfs_parent_name_irec {
	__uint64_t	p_ino;
	__uint32_t	p_gen;
	__uint32_t	p_diroffset;
	const char	*p_name;
	__uint32_t	p_namelen;
};
I think p_ino ought to be xfs_ino_t and not uint64_t, and p_diroffset ought to be xfs_dir2_dataptr_t instead of uint32_t.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
My goal at the moment is just to get it compiling again and finish out some of the sub routines that maintain it. It looks like it hasn't had much attention in a while, so I wanted to let people know the direction I'm planning to move in before I get too far in.
<nod>
Quoted 10 lines
Quoted 5 lines
If you're forward-porting that 17-patch set from Mark, I'd suggest first reading Dave's response to it - IIRC it amounted to a firm NAK. it also highlights the complexity of this undertaking, and may explain why nobody has gotten it done (yet) :)
The patches that came from me (to Allison) were last sent to me directly from Dave. I know he had some objections to the original design, but my understanding was that he had incorporated fixes for those issues in his modifications to Mark's original series (such as the xattr format and
From what I can tell, he seems to have taken Mark's patches to pass dir offsets back from the dirent manipulation functions and then started writing a simple implementation of stuffing the attrs into the attr tree.
Quoted 4 lines
whatnot), but the series wasn't completely done yet in terms of supporting all possible directory operations. (It's probably a good idea to review that old thread anyways just to confirm, unless Dave catches this and is able to chime in one way or the other.)
(Or jump in the fray out once he's back from sabbatical.)
Quoted 8 lines
I basically just forwarded ported Dave's patches to more recent kernels and added a couple minor fixes as I had combed through the existing code. I never really got to adding anything of substance before Allison volunteered to take over the series.
FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is.
Most of the users of xfs_trans_roll seem to be buried in the xattr code. For example, _attr_set can end up rolling a transaction after converting an attr fork from short format to leaf format before retrying the attr add operation, but we don't use log redo items (er, defer_ops items) to make sure log recovery will restart the attr add operation if we crash before the final _trans_commit after calling _attr_{leaf,node}_addname.

Quoted 9 lines

In the case of a regular xattr set operation this wouldn't have been a big deal because the fsetxattr call wouldn't have returned, so all the user could possibly see is an inode with a perhaps unnecessarily large xattr fork. Now that we need to set xattrs in the same transaction chain as a directory operation, it becomes very important that log recovery gain the ability to resume an xattr add operation no matter where the log stopped. Creating a directory can become this longwinded pile of updates:

Ok, thanks for the summary. So we have some transaction ordering down in the xattr code that might no longer be sufficient to provide atomicity in the context of parent pointers (via directory operations). I see that the current patch series increases the transaction size to accommodate parent pointers, but that doesn't help us deal with those situations where the transaction is rolled into a new one.

Quoted 11 lines

- Allocate directory block, map into data fork, chain rmap update, add directory entry, chain pptr update;
- Add rmap entry for new directory block (more chaining to fix freelist), finish first rmap update;
- Allocate xattr block for short->leaf conversion, map into attr fork, chain rmap update; chain xattr add operation;
- Add rmap entry for new xattr block (more chaining to fix freelist), finish second rmap update;
- Add xattr entry to file, finish xattr add operation;
- Finish pptr update.

Quoted 6 lines

Basically, I think someone's going to have to go audit all the uses of xfs_trans_roll in the attr code to figure out which operations need redo items, and how to cram everything toegether into a single xfs_defer_ops, rather than sprinkling them around the attr code like we do now, because redo items cannot be deferred from one defer_ops to another.

Indeed. I wonder whether we may have other options here as well, such as a higher level reordering/refactoring or allocation of necessary blocks in the directory code. I.e., I'm handwaving here, but perhaps following a model similar to how inode chunks are allocated in a separate transaction or directory operations are allowed in situations where an allocation is not (to limit deferring to where it is only necessary, for example). This would all be easier to reason about (IMO) with a functionally complete RFC series, though.

Brian

Quoted 60 lines

Quoted 2 lines
It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.
That's also a problem; unless someone can get Mark to supply them, we probably have to get someone to rewrite them. At a bare minimum I think we explicitly have to pass back a xfs_dir2_dataptr_t, not a bare uint32_t.
Quoted 4 lines
IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)
Agreed.
Quoted 33 lines
Brian
Quoted 31 lines
-Eric
Quoted 29 lines
Allison
Quoted 23 lines
Quoted 18 lines
Quoted 11 lines
I got in touch
with Brian Foster not too long ago and he had some code partially done from about a year or so ago (looks like it has patches from Dave Chinner and Mark Tinguely as well). So I am hoping to be able to use what we have so far to create something updated and finished out. I am still pretty new to the xfs code, so at the moment I am still just going through old discussion threads, and reviewing the patches. But for the most part I just wanted to see what people thought and get everyone on board with the idea. Suggestions and feedback are much appreciated. Thank you!!
It will be better if you can describe in more details if you have any specific goal with this, and/or what kind of improvement you expect to have with it, adding something new without a reason, is usually not well received.
Cheers
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Signature · 3 lines
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Quoted 8 lines
Quoted 4 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Brian Foster

2017-07-17 14:49 UTC #9 lore raw

In reply to Tinguely, Mark

Quoted 15 lines · Mark

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Darrick J. Wong
Sent: Friday, July 14, 2017 5:47 PM
To: Brian Foster <[email protected]>
Cc: Eric Sandeen <[email protected]>; Allison Henderson <[email protected]>; [email protected]; [email protected]
Subject: Re: Parent pointers
Quoted 4 lines · Brian Foster
Quoted 3 lines · Eric Sandeen
Quoted 2 lines · Allison Henderson
Quoted 1 line · Eric Sandeen
<parent inode pointers returneth messages deleted>

Quoted 8 lines

Quoted 2 lines
It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.
That's also a problem; unless someone can get Mark to supply them, we probably have to get someone to rewrite them. At a bare minimum I think we explicitly have to pass back a xfs_dir2_dataptr_t, not a bare uint32_t.
Me says:
The community can do what they want. If you want to start with that code, permission for S-O-B: me (snickers at the pun)

Thanks Mark. I guess we can add Mark's Signed-off-by to those patches he originally authored and that solves that problem. :)

Quoted 11 lines

Quoted 4 lines
IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)
No one is asking me but I will add, IMO (definitely disclaiming the storage group and HPE):
the PIP costs outweigh the benefits.
Adding the redundant filename for recovery and an occasional output of the path pushes the cost to be *totally unreasonable*.
If I had a say, I don't and won't, I would NAK any implementation that included it.
Implementing with extended attributes is the wrong, wrong, wrong choice. It was done for hysterical :) reasons and even that appeasement bit me in the asterisk.

Could you elaborate on this a bit? I haven't gone through all of the old discussions and it's been a while since I've even reviewed the recent code, but on a quick look at the links Darrick posted, it appears the old implementation incorporated the "first" parent pointer direct in the inode with a fallback to xattrs for inodes with multiple parents whereas the updated implementation uses xattrs outright (and as a result is able to incorporate the dentry name in the xattr). Is that an accurate summary?

If so, is the performance concern really the use of xattrs or the fact that the name is included (or some combination of both)? For example, would you consider a situation where pp's are entirely isolated to shortform xattrs much worse than the direct pp approach? It seems to me that the inline approach could still be considered as an optimization (i.e., maybe an incore cache) if performance does prove to be a problem. Maybe we could also consider something where (somehow, handwaving again ;) some number of pp xattrs are always shortform and thus remain within the inode.

IOW, I think performance is a valid concern, but this should probably 1.) follow correctness and 2.) be more dictated by testing (another really good reason to try and finish off the current rfc IMO) where we can consider various, potentially problematic situations. We're bound to have workloads where pretty much every file is hardlinked more than once (think glusterfs), for example.

Brian

Quoted 10 lines

Good luck.
--Mark Tinguely
not representing anyone but myself.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Mark Tinguely

2017-07-17 15:33 UTC #10 lore raw

Quoted 43 lines · Brian Foster

Quoted 11 lines · Mark
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Darrick J. Wong
Sent: Friday, July 14, 2017 5:47 PM
To: Brian Foster <[email protected]>
Cc: Eric Sandeen <[email protected]>; Allison Henderson <[email protected]>; [email protected]; [email protected]
Subject: Re: Parent pointers
Quoted 3 lines · Brian Foster
Quoted 2 lines · Eric Sandeen
Quoted 1 line · Allison Henderson
On 7/14/2017 7:04 AM, Eric Sandeen wrote:
Quoted 9 lines
<parent inode pointers returneth messages deleted>
Quoted 2 lines
It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.
That's also a problem; unless someone can get Mark to supply them, we probably have to get someone to rewrite them. At a bare minimum I think we explicitly have to pass back a xfs_dir2_dataptr_t, not a bare uint32_t.
Me says:
The community can do what they want. If you want to start with that code, permission for S-O-B: me (snickers at the pun)
Thanks Mark. I guess we can add Mark's Signed-off-by to those patches he originally authored and that solves that problem. :)
Quoted 10 lines
Quoted 4 lines
IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)
No one is asking me but I will add, IMO (definitely disclaiming the storage group and HPE):
the PIP costs outweigh the benefits.
Adding the redundant filename for recovery and an occasional output of the path pushes the cost to be *totally unreasonable*.
If I had a say, I don't and won't, I would NAK any implementation that included it.
Implementing with extended attributes is the wrong, wrong, wrong choice. It was done for hysterical :) reasons and even that appeasement bit me in the asterisk.
Could you elaborate on this a bit? I haven't gone through all of the old discussions and it's been a while since I've even reviewed the recent code, but on a quick look at the links Darrick posted, it appears the old implementation incorporated the "first" parent pointer direct in the inode with a fallback to xattrs for inodes with multiple parents whereas the updated implementation uses xattrs outright (and as a result is able to incorporate the dentry name in the xattr). Is that an accurate summary?

Quoted 9 lines

If so, is the performance concern really the use of xattrs or the fact that the name is included (or some combination of both)? For example, would you consider a situation where pp's are entirely isolated to shortform xattrs much worse than the direct pp approach? It seems to me that the inline approach could still be considered as an optimization (i.e., maybe an incore cache) if performance does prove to be a problem. Maybe we could also consider something where (somehow, handwaving again ;) some number of pp xattrs are always shortform and thus remain within the inode.

I disagree with both saving the name and using xattrs.

Quoted 10 lines

IOW, I think performance is a valid concern, but this should probably 1.) follow correctness and 2.) be more dictated by testing (another really good reason to try and finish off the current rfc IMO) where we can consider various, potentially problematic situations. We're bound to have workloads where pretty much every file is hardlinked more than once (think glusterfs), for example.
Brian

OMG! Sigh ...

Quoted 1 line

Quoted 1 line
Good luck.

Quoted 9 lines

Quoted 9 lines
--Mark Tinguely
not representing anyone but myself.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Dave Chinner

2017-07-17 22:14 UTC #13 lore raw

In reply to Brian Foster

Quoted 6 lines · Brian Foster

Quoted 5 lines · Darrick J. Wong
On Fri, Jul 14, 2017 at 03:07:41PM -0400, Brian Foster wrote:
From what I can tell, he seems to have taken Mark's patches to pass dir offsets back from the dirent manipulation functions and then started writing a simple implementation of stuffing the attrs into the attr tree.

Yup, that's the idea.

Quoted 21 lines

Quoted 11 lines
Quoted 3 lines
FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is.
Most of the users of xfs_trans_roll seem to be buried in the xattr code. For example, _attr_set can end up rolling a transaction after converting an attr fork from short format to leaf format before retrying the attr add operation, but we don't use log redo items (er, defer_ops items) to make sure log recovery will restart the attr add operation if we crash before the final _trans_commit after calling _attr_{leaf,node}_addname.
Quoted 9 lines
In the case of a regular xattr set operation this wouldn't have been a big deal because the fsetxattr call wouldn't have returned, so all the user could possibly see is an inode with a perhaps unnecessarily large xattr fork. Now that we need to set xattrs in the same transaction chain as a directory operation, it becomes very important that log recovery gain the ability to resume an xattr add operation no matter where the log stopped. Creating a directory can become this longwinded pile of updates:

Quoted 3 lines

Ok, thanks for the summary. So we have some transaction ordering down in the xattr code that might no longer be sufficient to provide atomicity in the context of parent pointers (via directory operations).

We already have xattr operations that are not atomic with directory entry creation - security xattrs (default ACL, etc). This can mean that after a crash and recovery, we have a newly created file without any security xattr on it.

This is a long standing problem - it bites SGI's HSM (DMF) all the time, so rather than fixing the problem 10+ years ago, they added a post-boot inode scan (via bulkstat) to find inodes that didn't have a DMF xattr.

The original parent pointer patchset also had this problem the moment xattrs were required (i.e. first hardlink) - the creation of the pp xattr was not atomic with the dirent creation. To solve this problem, I started by moving the xattr creation into the transaction context of the dirent.

Quoted 4 lines

I see that
the current patch series increases the transaction size to accommodate parent pointers, but that doesn't help us deal with those situations where the transaction is rolled into a new one.

Right, I hadn't got that far. The plan effective was to get a proof of concept running with the xattr creation within the directory creation xact context and get that working, knowing that it still wasn't properly atomic. This was done before we had deferred ops, and my thinking at the time was to add an intent/done pair of ops to tell recovery it needed to add an xattr to the dirent once the xattr was inside the dirent context. i.e.

- move xattr inside dirent ctx
- add pp intent
- roll transaction
- add pp xattr
- roll final xattr transaction rather than commit
- add pp done
- commit dir ctx transaction.

This doesn't help us with security context xattrs, though, so when deferred ops came along, it made much more sense to me to make the adding of an xattr a deferred op so we could use the same mechanism for adding all the required xattrs to the inode atomically and so have a mechanism for recovery to replay them appropriately.

Quoted 11 lines

Quoted 11 lines
- Allocate directory block, map into data fork, chain rmap update, add directory entry, chain pptr update;
- Add rmap entry for new directory block (more chaining to fix freelist), finish first rmap update;
- Allocate xattr block for short->leaf conversion, map into attr fork, chain rmap update; chain xattr add operation;
- Add rmap entry for new xattr block (more chaining to fix freelist), finish second rmap update;
- Add xattr entry to file, finish xattr add operation;
- Finish pptr update.

Quoted 5 lines

Quoted 5 lines
Basically, I think someone's going to have to go audit all the uses of xfs_trans_roll in the attr code to figure out which operations need redo items, and how to cram everything toegether into a single xfs_defer_ops, rather than sprinkling them around the attr code like we do now, because redo items cannot be deferred from one defer_ops to another.

Probably should handle that when we dup the transaction just before the roll, right?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]

Darrick J. Wong

2017-07-17 22:53 UTC #11 lore raw

In reply to Mark Tinguely

Quoted 44 lines · Mark Tinguely

Quoted 43 lines · Brian Foster
Quoted 11 lines · Mark
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Darrick J. Wong
Sent: Friday, July 14, 2017 5:47 PM
To: Brian Foster <[email protected]>
Cc: Eric Sandeen <[email protected]>; Allison Henderson <[email protected]>; [email protected]; [email protected]
Subject: Re: Parent pointers
Quoted 3 lines · Brian Foster
Quoted 2 lines · Eric Sandeen
Quoted 1 line · Allison Henderson
On 7/14/2017 7:04 AM, Eric Sandeen wrote:
Quoted 9 lines
<parent inode pointers returneth messages deleted>
Quoted 2 lines
It's something to keep in mind, in any event. IIRC there were also missing Signed-off-by's required for some of Mark's original patches.
That's also a problem; unless someone can get Mark to supply them, we probably have to get someone to rewrite them. At a bare minimum I think we explicitly have to pass back a xfs_dir2_dataptr_t, not a bare uint32_t.
Me says:
The community can do what they want. If you want to start with that code, permission for S-O-B: me (snickers at the pun)
Thanks Mark. I guess we can add Mark's Signed-off-by to those patches he originally authored and that solves that problem. :)
Quoted 10 lines
Quoted 4 lines
IMO, the best next step might be to just finish off the implementation as-is such that we could have a fairly functional RFC to put on the list and hash out whether there are in fact any remaining design hurdles, but others might have a different opinion on that. :)
No one is asking me but I will add, IMO (definitely disclaiming the storage group and HPE):
the PIP costs outweigh the benefits.
Adding the redundant filename for recovery and an occasional output of the path pushes the cost to be *totally unreasonable*.
If I had a say, I don't and won't, I would NAK any implementation that included it.
Implementing with extended attributes is the wrong, wrong, wrong choice. It was done for hysterical :) reasons and even that appeasement bit me in the asterisk.
Could you elaborate on this a bit? I haven't gone through all of the old discussions and it's been a while since I've even reviewed the recent code, but on a quick look at the links Darrick posted, it appears the old implementation incorporated the "first" parent pointer direct in the inode with a fallback to xattrs for inodes with multiple parents whereas the updated implementation uses xattrs outright (and as a result is able to incorporate the dentry name in the xattr). Is that an accurate summary?

Quoted 9 lines

Quoted 9 lines
If so, is the performance concern really the use of xattrs or the fact that the name is included (or some combination of both)? For example, would you consider a situation where pp's are entirely isolated to shortform xattrs much worse than the direct pp approach? It seems to me that the inline approach could still be considered as an optimization (i.e., maybe an incore cache) if performance does prove to be a problem. Maybe we could also consider something where (somehow, handwaving again ;) some number of pp xattrs are always shortform and thus remain within the inode.

I'd rather not do that, since a certain filesystem is, uh, extending its ability to store extended attributes (with biases to keep inline data and ACLs and such inside the inode). Now they have two different places where keys can hide and three places where values can get stuffed, and it's ... a complicated mess.

Better just to leave the existing da btree structure that we understand, unless we find that the cost is so outrageous that it becomes worth breaking all our brains. As it is, fixing the existing atomicity problems w.r.t. xattrs will be invasive enough.

Quoted 1 line

I disagree with both saving the name and using xattrs.

FWIW I disagree with /not/ storing the name, because my vision for parent pointers is to store backrefs that are rich enough that we can (ideally) reconstruct the damaged parts of the directory tree or (worst case) feed lost+found better names.

(Though, we have enough cooks now that I encourage everyone with differing goals to speak up.)

Quoted 6 lines

Quoted 6 lines
IOW, I think performance is a valid concern, but this should probably 1.) follow correctness and 2.) be more dictated by testing (another really good reason to try and finish off the current rfc IMO) where we can consider various, potentially problematic situations. We're bound to have workloads where pretty much every file is hardlinked more than once (think glusterfs), for example.

Agree.

Quoted 5 lines

Quoted 2 lines
Brian
OMG! Sigh ...
Quoted 1 line
Quoted 1 line
Good luck.
x2

Thanks!

--D

Quoted 15 lines

Quoted 9 lines
Quoted 9 lines
--Mark Tinguely
not representing anyone but myself.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Darrick J. Wong

2017-07-17 23:10 UTC #14 lore raw

In reply to Dave Chinner

Quoted 40 lines · Dave Chinner

Quoted 6 lines · Brian Foster
Quoted 5 lines · Darrick J. Wong
On Fri, Jul 14, 2017 at 03:07:41PM -0400, Brian Foster wrote:
From what I can tell, he seems to have taken Mark's patches to pass dir offsets back from the dirent manipulation functions and then started writing a simple implementation of stuffing the attrs into the attr tree.
Yup, that's the idea.
Quoted 21 lines
Quoted 11 lines
Quoted 3 lines
FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is.
Most of the users of xfs_trans_roll seem to be buried in the xattr code. For example, _attr_set can end up rolling a transaction after converting an attr fork from short format to leaf format before retrying the attr add operation, but we don't use log redo items (er, defer_ops items) to make sure log recovery will restart the attr add operation if we crash before the final _trans_commit after calling _attr_{leaf,node}_addname.
Quoted 9 lines
In the case of a regular xattr set operation this wouldn't have been a big deal because the fsetxattr call wouldn't have returned, so all the user could possibly see is an inode with a perhaps unnecessarily large xattr fork. Now that we need to set xattrs in the same transaction chain as a directory operation, it becomes very important that log recovery gain the ability to resume an xattr add operation no matter where the log stopped. Creating a directory can become this longwinded pile of updates:
Quoted 3 lines
Ok, thanks for the summary. So we have some transaction ordering down in the xattr code that might no longer be sufficient to provide atomicity in the context of parent pointers (via directory operations).
We already have xattr operations that are not atomic with directory entry creation - security xattrs (default ACL, etc). This can mean that after a crash and recovery, we have a newly created file without any security xattr on it.

Quoted 4 lines

This is a long standing problem - it bites SGI's HSM (DMF) all the time, so rather than fixing the problem 10+ years ago, they added a post-boot inode scan (via bulkstat) to find inodes that didn't have a DMF xattr.

<nod>

That reminds me, Ted was asking what _check_xfs_test_fs (in xfstests) does. Judging from the nonexistent xfs_repair_ipaths command, I'm guessing that program fixed up pptr xattrs if they were broken/missing/whatever?

Quoted 27 lines

The original parent pointer patchset also had this problem the moment xattrs were required (i.e. first hardlink) - the creation of the pp xattr was not atomic with the dirent creation. To solve this problem, I started by moving the xattr creation into the transaction context of the dirent.
Quoted 4 lines
I see that
the current patch series increases the transaction size to accommodate parent pointers, but that doesn't help us deal with those situations where the transaction is rolled into a new one.
Right, I hadn't got that far. The plan effective was to get a proof of concept running with the xattr creation within the directory creation xact context and get that working, knowing that it still wasn't properly atomic. This was done before we had deferred ops, and my thinking at the time was to add an intent/done pair of ops to tell recovery it needed to add an xattr to the dirent once the xattr was inside the dirent context. i.e.
- move xattr inside dirent ctx
- add pp intent
- roll transaction
- add pp xattr
- roll final xattr transaction rather than commit
- add pp done
- commit dir ctx transaction.

Quoted 5 lines

This doesn't help us with security context xattrs, though, so when deferred ops came along, it made much more sense to me to make the adding of an xattr a deferred op so we could use the same mechanism for adding all the required xattrs to the inode atomically and so have a mechanism for recovery to replay them appropriately.

<nod>

Quoted 11 lines

Quoted 11 lines
Quoted 11 lines
- Allocate directory block, map into data fork, chain rmap update, add directory entry, chain pptr update;
- Add rmap entry for new directory block (more chaining to fix freelist), finish first rmap update;
- Allocate xattr block for short->leaf conversion, map into attr fork, chain rmap update; chain xattr add operation;
- Add rmap entry for new xattr block (more chaining to fix freelist), finish second rmap update;
- Add xattr entry to file, finish xattr add operation;
- Finish pptr update.

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
Basically, I think someone's going to have to go audit all the uses of xfs_trans_roll in the attr code to figure out which operations need redo items, and how to cram everything toegether into a single xfs_defer_ops, rather than sprinkling them around the attr code like we do now, because redo items cannot be deferred from one defer_ops to another.
Probably should handle that when we dup the transaction just before the roll, right?

Something like that. Ideally, I think I'd like to have two xattr redo
items: "add key => value" and "remove key", where "add key" would first ensure xattr space, roll, and then actually add the attr & value. A big problem with that scheme is that we then have to log the value, which could be ... tough.

--D

Quoted 11 lines

Cheers,

Dave.

Signature · 6 lines

Dave Chinner
[email protected]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner

2017-07-18 00:11 UTC #15 lore raw

Quoted 46 lines · Darrick J. Wong

Quoted 40 lines · Dave Chinner
Quoted 6 lines · Brian Foster
Quoted 5 lines · Darrick J. Wong
On Fri, Jul 14, 2017 at 03:07:41PM -0400, Brian Foster wrote:
From what I can tell, he seems to have taken Mark's patches to pass dir offsets back from the dirent manipulation functions and then started writing a simple implementation of stuffing the attrs into the attr tree.
Yup, that's the idea.
Quoted 21 lines
Quoted 11 lines
Quoted 3 lines
FWIW, I think there was some mention of porting some of the operations over to the deferred ops infrastructure, but it's not clear to me off the top of my head how important (or appropriate) that is.
Most of the users of xfs_trans_roll seem to be buried in the xattr code. For example, _attr_set can end up rolling a transaction after converting an attr fork from short format to leaf format before retrying the attr add operation, but we don't use log redo items (er, defer_ops items) to make sure log recovery will restart the attr add operation if we crash before the final _trans_commit after calling _attr_{leaf,node}_addname.
Quoted 9 lines
In the case of a regular xattr set operation this wouldn't have been a big deal because the fsetxattr call wouldn't have returned, so all the user could possibly see is an inode with a perhaps unnecessarily large xattr fork. Now that we need to set xattrs in the same transaction chain as a directory operation, it becomes very important that log recovery gain the ability to resume an xattr add operation no matter where the log stopped. Creating a directory can become this longwinded pile of updates:
Quoted 3 lines
Ok, thanks for the summary. So we have some transaction ordering down in the xattr code that might no longer be sufficient to provide atomicity in the context of parent pointers (via directory operations).
We already have xattr operations that are not atomic with directory entry creation - security xattrs (default ACL, etc). This can mean that after a crash and recovery, we have a newly created file without any security xattr on it.
Quoted 4 lines
This is a long standing problem - it bites SGI's HSM (DMF) all the time, so rather than fixing the problem 10+ years ago, they added a post-boot inode scan (via bulkstat) to find inodes that didn't have a DMF xattr.

Quoted 6 lines

<nod>
That reminds me, Ted was asking what _check_xfs_test_fs (in xfstests) does. Judging from the nonexistent xfs_repair_ipaths command, I'm guessing that program fixed up pptr xattrs if they were broken/missing/whatever?

I can't remember - there was an aborted parent pointer implementation committed to Irix 6.5.27 that had some userspace tools written for it. That implementation was aborted because the inode generation number was not in the on-disk PP format and so lookups would find the wrong parent inode if it raced with unlinks. ISTR that xfs_repair_ipaths was a standalone dev tool used to check/rebuild parent paths as that functionality hadn't been added to xfs_repair yet...

Quoted 17 lines

Quoted 17 lines
Quoted 16 lines
Quoted 16 lines
- Allocate directory block, map into data fork, chain rmap update, add directory entry, chain pptr update;
- Add rmap entry for new directory block (more chaining to fix freelist), finish first rmap update;
- Allocate xattr block for short->leaf conversion, map into attr fork, chain rmap update; chain xattr add operation;
- Add rmap entry for new xattr block (more chaining to fix freelist), finish second rmap update;
- Add xattr entry to file, finish xattr add operation;
- Finish pptr update.
Basically, I think someone's going to have to go audit all the uses of xfs_trans_roll in the attr code to figure out which operations need redo items, and how to cram everything toegether into a single xfs_defer_ops, rather than sprinkling them around the attr code like we do now, because redo items cannot be deferred from one defer_ops to another.

Quoted 8 lines

Quoted 2 lines
Probably should handle that when we dup the transaction just before the roll, right?
Something like that. Ideally, I think I'd like to have two xattr redo
items: "add key => value" and "remove key", where "add key" would first ensure xattr space, roll, and then actually add the attr & value. A big problem with that scheme is that we then have to log the value, which could be ... tough.

Well, there's a couple of things to remember here:
1. we already log the attribute value in the modified buffer if it is inserted inline.
2. we don't log the attribute values for remote attributes, but instead do a sync write after allocation but before we commit the transaction.

For parent pointers, the attribute value is always going to be less than 255 bytes, so it will always be inline and so logging the name/value pair in the redo item is, IMO, not going to be an issue for parent pointers.

For security attributes it might be a different story - how big do default ACLs and security labels get, anyway? Maybe one we can simply suck it up (i.e. burn log bandwidth) given how rare huge attributes tend to be?

Cheers,

Dave.

Signature · 2 lines

Dave Chinner
[email protected]