Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1592346 > unrolled thread

Re: [RFC PATCH v1 11/30] fs: new API for handling i_version

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH v1 11/30] fs: new API for handling i_version NeilBrown <neil@brown.name> - 2017-03-04 01:00 +0100
    Re: [RFC PATCH v1 11/30] fs: new API for handling i_version Jeff Layton <jlayton@redhat.com> - 2017-03-04 03:00 +0100

#1592346 — Re: [RFC PATCH v1 11/30] fs: new API for handling i_version

[Multipart message — attachments visible in raw view] — view raw

On Wed, Dec 21 2016, Jeff Layton wrote:

> We already have inode_inc_iversion. Add inode_set_iversion,
> inode_get_iversion, inode_cmp_iversion and inode_iversion_need_inc.

This list of added interfaces is incomplete.
And some of these interfaces could really use some justification up
front.

You later add a "force" parameter to inode_inc_version.
Why not do that up front here?

> +
> +/**
> + * inode_get_iversion - read i_version for later use
> + * @inode: inode from which i_version should be read
> + *
> + * Read the inode i_version counter. This should be used by callers that wish
> + * to store the returned i_version for later comparison.
> + */
> +static inline u64
> +inode_get_iversion(const struct inode *inode)
> +{
> +	return inode_get_iversion_raw(inode);
> +}

I don't understand why this can use the _raw version rather than the
_read version.
Surely you need to know about any changes after this read.

Thanks,
NeilBrown

[toc] | [next] | [standalone]

#1592376

On Sat, 2017-03-04 at 10:55 +1100, NeilBrown wrote:
> On Wed, Dec 21 2016, Jeff Layton wrote:
> 
> > We already have inode_inc_iversion. Add inode_set_iversion,
> > inode_get_iversion, inode_cmp_iversion and inode_iversion_need_inc.
> 
> This list of added interfaces is incomplete.
> And some of these interfaces could really use some justification up
> front.
> 
> You later add a "force" parameter to inode_inc_version.
> Why not do that up front here?
> 

First, thanks to you and Bruce for having a look.

Yes, it's clear from this and the earlier emails that I didn't do
enough documentation and explanation. I'll plan to respin this when I
get a chance, and lay out the justification and discussion a bit more.

I'll also make sure the not change the API midstream like this when I
respin.

> > +
> > +/**
> > + * inode_get_iversion - read i_version for later use
> > + * @inode: inode from which i_version should be read
> > + *
> > + * Read the inode i_version counter. This should be used by callers that wish
> > + * to store the returned i_version for later comparison.
> > + */
> > +static inline u64
> > +inode_get_iversion(const struct inode *inode)
> > +{
> > +	return inode_get_iversion_raw(inode);
> > +}
> 
> I don't understand why this can use the _raw version rather than the
> _read version.
> Surely you need to know about any changes after this read.
> 

The approach here was to convert everything to a new API and have it
work much like the code works today and then to morph it into something
that only conditionally bumps the counter.

In the later implementation, yes you do need to know about changes
after the read, but in the initial implementation it doesn't matter
since the counter is bumped on every change anyway.

I'll try to do a better job laying out this rationale in follow-on
postings.
-- 
Jeff Layton <jlayton@redhat.com>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web