* Re: ext4: cache all of an extent tree's leaf block upon reading [not found] <20130905013848.42DD7660EE1@gitolite.kernel.org> @ 2013-09-05 14:37 ` Dave Jones 2013-09-05 14:53 ` Theodore Ts'o 0 siblings, 1 reply; 4+ messages in thread From: Dave Jones @ 2013-09-05 14:37 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: tytso On Thu, Sep 05, 2013 at 01:38:48AM +0000, Linux Kernel wrote: > Gitweb: http://git.kernel.org/linus/;a=commit;h=107a7bd31ac003e42c0f966aa8e5b26947de6024 > Commit: 107a7bd31ac003e42c0f966aa8e5b26947de6024 > Parent: 3be78c73179c9347bdc0a92b2898063bd2300ff7 > Author: Theodore Ts'o <tytso@mit.edu> > AuthorDate: Fri Aug 16 21:23:41 2013 -0400 > Committer: Theodore Ts'o <tytso@mit.edu> > CommitDate: Fri Aug 16 21:23:41 2013 -0400 > > ext4: cache all of an extent tree's leaf block upon reading > + * ext4_es_cache_extent() inserts information into the extent status > + * tree if and only if there isn't information about the range in > + * question already. > + */ > +void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk, > + ext4_lblk_t len, ext4_fsblk_t pblk, > + unsigned int status) > +{ > + struct extent_status *es; > + struct extent_status newes; > + ext4_lblk_t end = lblk + len - 1; > + > + newes.es_lblk = lblk; > + newes.es_len = len; > + ext4_es_store_pblock(&newes, pblk); ext4_es_store_pblock or's the pblk with the existing contents of the struct member. (albeit masked with ES_MASK) Should there be a newes.es_pblk = 0; up there too ? It seems like if the stack happened to contain any of ES_WRITTEN | ES_UNWRITTEN | ES_DELAYED | ES_HOLE then it could leak through into the new extent status. Dave ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ext4: cache all of an extent tree's leaf block upon reading 2013-09-05 14:37 ` ext4: cache all of an extent tree's leaf block upon reading Dave Jones @ 2013-09-05 14:53 ` Theodore Ts'o 2013-09-05 15:14 ` Dave Jones 0 siblings, 1 reply; 4+ messages in thread From: Theodore Ts'o @ 2013-09-05 14:53 UTC (permalink / raw) To: Dave Jones, Linux Kernel Mailing List On Thu, Sep 05, 2013 at 10:37:49AM -0400, Dave Jones wrote: > > +void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk, > > + ext4_lblk_t len, ext4_fsblk_t pblk, > > + unsigned int status) > > +{ > > + struct extent_status *es; > > + struct extent_status newes; > > + ext4_lblk_t end = lblk + len - 1; > > + > > + newes.es_lblk = lblk; > > + newes.es_len = len; > > + ext4_es_store_pblock(&newes, pblk); > > > ext4_es_store_pblock or's the pblk with the existing contents of the struct member. > (albeit masked with ES_MASK) > > Should there be a > > newes.es_pblk = 0; > > up there too ? The next line after ext4_es_store_pblock() is: ext4_es_store_status(&newes, status); This will set remaining ES_WRITTEN | ES_UNWRITTEN... bits. So the only reason to add a line explicitly setting es_pblk to zero would be to suppress a warning from some insufficiently smart static code analysis tool. I didn't see a warning from gcc, but it's possible that this is something which is causing Coverity or some other code scanner heartburn. Cheers, - Ted ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ext4: cache all of an extent tree's leaf block upon reading 2013-09-05 14:53 ` Theodore Ts'o @ 2013-09-05 15:14 ` Dave Jones 2013-09-05 19:08 ` Theodore Ts'o 0 siblings, 1 reply; 4+ messages in thread From: Dave Jones @ 2013-09-05 15:14 UTC (permalink / raw) To: Theodore Ts'o, Linux Kernel Mailing List On Thu, Sep 05, 2013 at 10:53:34AM -0400, Theodore Ts'o wrote: > > ext4_es_store_pblock or's the pblk with the existing contents of the struct member. > > (albeit masked with ES_MASK) > > > > Should there be a > > > > newes.es_pblk = 0; > > > > up there too ? > > The next line after ext4_es_store_pblock() is: > > ext4_es_store_status(&newes, status); > > This will set remaining ES_WRITTEN | ES_UNWRITTEN... bits. > > So the only reason to add a line explicitly setting es_pblk to zero > would be to suppress a warning from some insufficiently smart static > code analysis tool. I didn't see a warning from gcc, but it's > possible that this is something which is causing Coverity or some > other code scanner heartburn. Yep, that's what picked it up. I'll add a 'not a bug' annotation to stop it getting flagged again. This was the only ext* issue that Coverity picked up from yesterdays merge btw, which I guess is good news ;) Dave ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: ext4: cache all of an extent tree's leaf block upon reading 2013-09-05 15:14 ` Dave Jones @ 2013-09-05 19:08 ` Theodore Ts'o 0 siblings, 0 replies; 4+ messages in thread From: Theodore Ts'o @ 2013-09-05 19:08 UTC (permalink / raw) To: Dave Jones, Linux Kernel Mailing List On Thu, Sep 05, 2013 at 11:14:57AM -0400, Dave Jones wrote: > > So the only reason to add a line explicitly setting es_pblk to zero > > would be to suppress a warning from some insufficiently smart static > > code analysis tool. I didn't see a warning from gcc, but it's > > possible that this is something which is causing Coverity or some > > other code scanner heartburn. > > Yep, that's what picked it up. I'll add a 'not a bug' annotation to stop > it getting flagged again. This was the only ext* issue that Coverity > picked up from yesterdays merge btw, which I guess is good news ;) Indeed. Hmm... I could add a new inline function "ext4_es_store_pblock_status()" which sets both parts of the es_pblk word at once, and which doesn't depend looking at its original value at all. I doubt we would never measure a difference in performance, but in theory it would be more efficient. And if it eliminates a potential static code analysis complaint, maybe the two justifications is good enough to add the new function. Thanks for checking the coverity results!! - Ted ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-05 19:08 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20130905013848.42DD7660EE1@gitolite.kernel.org>
2013-09-05 14:37 ` ext4: cache all of an extent tree's leaf block upon reading Dave Jones
2013-09-05 14:53 ` Theodore Ts'o
2013-09-05 15:14 ` Dave Jones
2013-09-05 19:08 ` Theodore Ts'o
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox