Re: [PATCH RFC PKS/PMEM 22/58] fs/f2fs: Utilize new kmap_thread()
On Fri, 2020-10-09 at 14:34 -0700, Eric Biggers wrote:
> On Fri, Oct 09, 2020 at 12:49:57PM -0700, ira.we...@intel.com wrote:
> > From: Ira Weiny
> >
> > The kmap() calls in this FS are localized to a single thread. To
> > avoid the over head of global PKRS updates use the new
> > kmap_thread() call.
> >
> > Cc: Jaegeuk Kim
> > Cc: Chao Yu
> > Signed-off-by: Ira Weiny
> > ---
> > fs/f2fs/f2fs.h | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index d9e52a7f3702..ff72a45a577e 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2410,12 +2410,12 @@ static inline struct page
> > *f2fs_pagecache_get_page(
> >
> > static inline void f2fs_copy_page(struct page *src, struct page
> > *dst)
> > {
> > - char *src_kaddr = kmap(src);
> > - char *dst_kaddr = kmap(dst);
> > + char *src_kaddr = kmap_thread(src);
> > + char *dst_kaddr = kmap_thread(dst);
> >
> > memcpy(dst_kaddr, src_kaddr, PAGE_SIZE);
> > - kunmap(dst);
> > - kunmap(src);
> > + kunmap_thread(dst);
> > + kunmap_thread(src);
> > }
>
> Wouldn't it make more sense to switch cases like this to
> kmap_atomic()?
> The pages are only mapped to do a memcpy(), then they're immediately
> unmapped.
On a VIPT/VIVT architecture, this is horrendously wasteful. You're
taking something that was mapped at colour c_src mapping it to a new
address src_kaddr, which is likely a different colour and necessitates
flushing the original c_src, then you copy it to dst_kaddr, which is
also likely a different colour from c_dst, so dst_kaddr has to be
flushed on kunmap and c_dst has to be invalidated on kmap. What we
should have is an architectural primitive for doing this, something
like kmemcopy_arch(dst, src). PIPT architectures can implement it as
the above (possibly losing kmap if they don't need it) but VIPT/VIVT
architectures can set up a correctly coloured mapping so they can
simply copy from c_src to c_dst without any need to flush and the data
arrives cache hot at c_dst.
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > clang has a number of useful, new warnings see > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > Please get your IT department to remove that stupidity. If you > can't, please send email from a non-Red Hat email address. Actually, the problem is at Oracle's end somewhere in the ocfs2 list ... if you could fix it, that would be great. The usual real mailing lists didn't get this transformation https://lore.kernel.org/bpf/20201017160928.12698-1-t...@redhat.com/ but the ocfs2 list archive did: https://oss.oracle.com/pipermail/ocfs2-devel/2020-October/015330.html I bet Oracle IT has put some spam filter on the list that mangles URLs this way. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [Ocfs2-devel] [RFC] treewide: cleanup unreachable breaks
On Sun, 2020-10-18 at 20:16 +0100, Matthew Wilcox wrote: > On Sun, Oct 18, 2020 at 12:13:35PM -0700, James Bottomley wrote: > > On Sun, 2020-10-18 at 19:59 +0100, Matthew Wilcox wrote: > > > On Sat, Oct 17, 2020 at 09:09:28AM -0700, t...@redhat.com wrote: > > > > clang has a number of useful, new warnings see > > > > https://urldefense.com/v3/__https://clang.llvm.org/docs/DiagnosticsReference.html__;!!GqivPVa7Brio!Krxz78O3RKcB9JBMVo_F98FupVhj_jxX60ddN6tKGEbv_cnooXc1nnBmchm-e_O9ieGnyQ$ > > > > > > > > > > Please get your IT department to remove that stupidity. If you > > > can't, please send email from a non-Red Hat email address. > > > > Actually, the problem is at Oracle's end somewhere in the ocfs2 > > list ... if you could fix it, that would be great. The usual real > > mailing lists didn't get this transformation > > > > https://lore.kernel.org/bpf/20201017160928.12698-1-t...@redhat.com/ > > > > but the ocfs2 list archive did: > > > > https://oss.oracle.com/pipermail/ocfs2-devel/2020-October/015330.html > > > > I bet Oracle IT has put some spam filter on the list that mangles > > URLs this way. > > *sigh*. I'm sure there's a way. I've raised it with someone who > should be able to fix it. As someone who works for IBM I can only say I feel your pain ... James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 08:17 -0800, Kees Cook wrote: > On Fri, Nov 20, 2020 at 11:51:42AM -0800, Jakub Kicinski wrote: > > On Fri, 20 Nov 2020 11:30:40 -0800 Kees Cook wrote: > > > On Fri, Nov 20, 2020 at 10:53:44AM -0800, Jakub Kicinski wrote: > > > > On Fri, 20 Nov 2020 12:21:39 -0600 Gustavo A. R. Silva wrote: > > > > > This series aims to fix almost all remaining fall-through > > > > > warnings in order to enable -Wimplicit-fallthrough for Clang. > > > > > > > > > > In preparation to enable -Wimplicit-fallthrough for Clang, > > > > > explicitly add multiple break/goto/return/fallthrough > > > > > statements instead of just letting the code fall through to > > > > > the next case. > > > > > > > > > > Notice that in order to enable -Wimplicit-fallthrough for > > > > > Clang, this change[1] is meant to be reverted at some point. > > > > > So, this patch helps to move in that direction. > > > > > > > > > > Something important to mention is that there is currently a > > > > > discrepancy between GCC and Clang when dealing with switch > > > > > fall-through to empty case statements or to cases that only > > > > > contain a break/continue/return statement[2][3][4]. > > > > > > > > Are we sure we want to make this change? Was it discussed > > > > before? > > > > > > > > Are there any bugs Clangs puritanical definition of fallthrough > > > > helped find? > > > > > > > > IMVHO compiler warnings are supposed to warn about issues that > > > > could be bugs. Falling through to default: break; can hardly be > > > > a bug?! > > > > > > It's certainly a place where the intent is not always clear. I > > > think this makes all the cases unambiguous, and doesn't impact > > > the machine code, since the compiler will happily optimize away > > > any behavioral redundancy. > > > > If none of the 140 patches here fix a real bug, and there is no > > change to machine code then it sounds to me like a W=2 kind of a > > warning. > > FWIW, this series has found at least one bug so far: > https://lore.kernel.org/lkml/CAFCwf11izHF=g1mGry1fE5kvFFFrxzhPSM6qKAO8gxSp=kr...@mail.gmail.com/ Well, it's a problem in an error leg, sure, but it's not a really compelling reason for a 141 patch series, is it? All that fixing this error will do is get the driver to print "oh dear there's a problem" under four more conditions than it previously did. We've been at this for three years now with nearly a thousand patches, firstly marking all the fall throughs with /* fall through */ and later changing it to fallthrough. At some point we do have to ask if the effort is commensurate with the protection afforded. Please tell me our reward for all this effort isn't a single missing error print. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > Please tell me our reward for all this effort isn't a single > > missing error print. > > There were quite literally dozens of logical defects found > by the fallthrough additions. Very few were logging only. So can you give us the best examples (or indeed all of them if someone is keeping score)? hopefully this isn't a US election situation ... James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 11:22 -0800, Joe Perches wrote: > On Sun, 2020-11-22 at 11:12 -0800, James Bottomley wrote: > > On Sun, 2020-11-22 at 10:25 -0800, Joe Perches wrote: > > > On Sun, 2020-11-22 at 10:21 -0800, James Bottomley wrote: > > > > Please tell me our reward for all this effort isn't a single > > > > missing error print. > > > > > > There were quite literally dozens of logical defects found > > > by the fallthrough additions. Very few were logging only. > > > > So can you give us the best examples (or indeed all of them if > > someone is keeping score)? hopefully this isn't a US election > > situation ... > > Gustavo? Are you running for congress now? > > https://lwn.net/Articles/794944/ That's 21 reported fixes of which about 50% seem to produce no change in code behaviour at all, a quarter seem to have no user visible effect with the remaining quarter producing unexpected errors on obscure configuration parameters, which is why no-one really noticed them before. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Sun, 2020-11-22 at 21:35 +0100, Miguel Ojeda wrote: > On Sun, Nov 22, 2020 at 7:22 PM James Bottomley > wrote: > > Well, it's a problem in an error leg, sure, but it's not a really > > compelling reason for a 141 patch series, is it? All that fixing > > this error will do is get the driver to print "oh dear there's a > > problem" under four more conditions than it previously did. > > > > We've been at this for three years now with nearly a thousand > > patches, firstly marking all the fall throughs with /* fall through > > */ and later changing it to fallthrough. At some point we do have > > to ask if the effort is commensurate with the protection > > afforded. Please tell me our reward for all this effort isn't a > > single missing error print. > > It isn't that much effort, isn't it? Well, it seems to be three years of someone's time plus the maintainer review time and series disruption of nearly a thousand patches. Let's be conservative and assume the producer worked about 30% on the series and it takes about 5-10 minutes per patch to review, merge and for others to rework existing series. So let's say it's cost a person year of a relatively junior engineer producing the patches and say 100h of review and application time. The latter is likely the big ticket item because it's what we have in least supply in the kernel (even though it's 20x vs the producer time). > Plus we need to take into account the future mistakes that it might > prevent, too. So even if there were zero problems found so far, it is > still a positive change. Well, the question I was asking is if it's worth the cost which I've tried to outline above. > I would agree if these changes were high risk, though; but they are > almost trivial. It's not about the risk of the changes it's about the cost of implementing them. Even if you discount the producer time (which someone gets to pay for, and if I were the engineering manager, I'd be unhappy about), the review/merge/rework time is pretty significant in exchange for six minor bug fixes. Fine, when a new compiler warning comes along it's certainly reasonable to see if we can benefit from it and the fact that the compiler people think it's worthwhile is enough evidence to assume this initially. But at some point you have to ask whether that assumption is supported by the evidence we've accumulated over the time we've been using it. And if the evidence doesn't support it perhaps it is time to stop the experiment. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 15:19 +0100, Miguel Ojeda wrote: > On Sun, Nov 22, 2020 at 11:36 PM James Bottomley > wrote: > > Well, it seems to be three years of someone's time plus the > > maintainer review time and series disruption of nearly a thousand > > patches. Let's be conservative and assume the producer worked > > about 30% on the series and it takes about 5-10 minutes per patch > > to review, merge and for others to rework existing series. So > > let's say it's cost a person year of a relatively junior engineer > > producing the patches and say 100h of review and application > > time. The latter is likely the big ticket item because it's what > > we have in least supply in the kernel (even though it's 20x vs the > > producer time). > > How are you arriving at such numbers? It is a total of ~200 trivial > lines. Well, I used git. It says that as of today in Linus' tree we have 889 patches related to fall throughs and the first series went in in october 2017 ... ignoring a couple of outliers back to February. > > It's not about the risk of the changes it's about the cost of > > implementing them. Even if you discount the producer time (which > > someone gets to pay for, and if I were the engineering manager, I'd > > be unhappy about), the review/merge/rework time is pretty > > significant in exchange for six minor bug fixes. Fine, when a new > > compiler warning comes along it's certainly reasonable to see if we > > can benefit from it and the fact that the compiler people think > > it's worthwhile is enough evidence to assume this initially. But > > at some point you have to ask whether that assumption is supported > > by the evidence we've accumulated over the time we've been using > > it. And if the evidence doesn't support it perhaps it is time to > > stop the experiment. > > Maintainers routinely review 1-line trivial patches, not to mention > internal API changes, etc. We're also complaining about the inability to recruit maintainers: https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ And burn out: http://antirez.com/news/129 The whole crux of your argument seems to be maintainers' time isn't important so we should accept all trivial patches ... I'm pushing back on that assumption in two places, firstly the valulessness of the time and secondly that all trivial patches are valuable. > If some company does not want to pay for that, that's fine, but they > don't get to be maintainers and claim `Supported`. What I'm actually trying to articulate is a way of measuring value of the patch vs cost ... it has nothing really to do with who foots the actual bill. One thesis I'm actually starting to formulate is that this continual devaluing of maintainers is why we have so much difficulty keeping and recruiting them. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 000/141] Fix fall-through warnings for Clang
On Mon, 2020-11-23 at 19:56 +0100, Miguel Ojeda wrote: > On Mon, Nov 23, 2020 at 4:58 PM James Bottomley > wrote: > > Well, I used git. It says that as of today in Linus' tree we have > > 889 patches related to fall throughs and the first series went in > > in october 2017 ... ignoring a couple of outliers back to February. > > I can see ~10k insertions over ~1k commits and 15 years that mention > a fallthrough in the entire repo. That is including some commits > (like the biggest one, 960 insertions) that have nothing to do with C > fallthrough. A single kernel release has an order of magnitude more > changes than this... > > But if we do the math, for an author, at even 1 minute per line > change and assuming nothing can be automated at all, it would take 1 > month of work. For maintainers, a couple of trivial lines is noise > compared to many other patches. So you think a one line patch should take one minute to produce ... I really don't think that's grounded in reality. I suppose a one line patch only takes a minute to merge with b4 if no-one reviews or tests it, but that's not really desirable. > In fact, this discussion probably took more time than the time it > would take to review the 200 lines. :-) I'm framing the discussion in terms of the whole series of changes we have done for fall through, both what's in the tree currently (889 patches) both in terms of the produce and the consumer. That's what I used for my figures for cost. > > We're also complaining about the inability to recruit maintainers: > > > > https://www.theregister.com/2020/06/30/hard_to_find_linux_maintainers_says_torvalds/ > > > > And burn out: > > > > http://antirez.com/news/129 > > Accepting trivial and useful 1-line patches Part of what I'm trying to measure is the "and useful" bit because that's not a given. > is not what makes a voluntary maintainer quit... so the proverb "straw which broke the camel's back" uniquely doesn't apply to maintainers > Thankless work with demanding deadlines is. That's another potential reason, but it doesn't may other reasons less valid. > > The whole crux of your argument seems to be maintainers' time isn't > > important so we should accept all trivial patches > > I have not said that, at all. In fact, I am a voluntary one and I > welcome patches like this. It takes very little effort on my side to > review and it helps the kernel overall. Well, you know, subsystems are very different in terms of the amount of patches a maintainer has to process per release cycle of the kernel. If a maintainer is close to capacity, additional patches, however trivial, become a problem. If a maintainer has spare cycles, trivial patches may look easy. > Paid maintainers are the ones that can take care of big > features/reviews. > > > What I'm actually trying to articulate is a way of measuring value > > of the patch vs cost ... it has nothing really to do with who foots > > the actual bill. > > I understand your point, but you were the one putting it in terms of > a junior FTE. No, I evaluated the producer side in terms of an FTE. What we're mostly arguing about here is the consumer side: the maintainers and people who have to rework their patch sets. I estimated that at 100h. > In my view, 1 month-work (worst case) is very much worth > removing a class of errors from a critical codebase. > > > One thesis I'm actually starting to formulate is that this > > continual devaluing of maintainers is why we have so much > > difficulty keeping and recruiting them. > > That may very well be true, but I don't feel anybody has devalued > maintainers in this discussion. You seem to be saying that because you find it easy to merge trivial patches, everyone should. I'm reminded of a friend long ago who thought being a Tees River Pilot was a sinecure because he could navigate the Tees blindfold. What he forgot, of course, is that just because it's easy with a trawler doesn't mean it's easy with an oil tanker. In fact it takes longer to qualify as a Tees River Pilot than it does to get a PhD. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 08/15] staging: unisys: visorhba: use sg helper to operate sgl
On Thu, 2019-06-13 at 12:16 +0200, Greg Kroah-Hartman wrote: > On Thu, Jun 13, 2019 at 06:04:11PM +0800, Ming Lei wrote: > > On Thu, Jun 13, 2019 at 11:52:14AM +0200, Greg Kroah-Hartman wrote: > > > On Thu, Jun 13, 2019 at 03:13:28PM +0800, Ming Lei wrote: > > > > The current way isn't safe for chained sgl, so use sg helper to > > > > operate sgl. > > > > > > I can not make any sense out of this changelog. > > > > > > What "isn't safe"? What is a "sgl"? > > > > sgl is 'scatterlist' in kernel, and several linear sgl can be > > chained together, so accessing the sgl in linear way may see a > > chained sg, which is like a link pointer, then may cause trouble > > for driver. > > What kind of "trouble"? Is this a bug fix that needs to be > backported to stable kernels? How can this be triggered? OK, stop. I haven't seen the commit log since the original hasn't appeared on the list yet, but the changelog needs to say something like this to prevent questions like the above, as I asked in the last review. Please make it something like this: --- Scsi MQ makes a large static allocation for the first scatter gather list chunk for the driver to use. This is a performance headache we'd like to fix by reducing the size of the allocation to a 2 element array. Doing this will break the current guarantee that any driver using SG_ALL doesn't need to use the scatterlist iterators and can get away with directly dereferencing the array. Thus we need to update all drivers to use the scatterlist iterators and remove direct indexing of the scatterlist array before reducing the initial scatterlist allocation size in SCSI. --- James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: exfat filesystem
On Tue, 2019-07-09 at 08:48 -0700, Matthew Wilcox wrote: > On Tue, Jul 09, 2019 at 11:30:39AM -0400, Theodore Ts'o wrote: > > On Tue, Jul 09, 2019 at 04:21:36AM -0700, Matthew Wilcox wrote: > > > How does > > > https://www.zdnet.com/article/microsoft-open-sources-its-entire-p > > > atent-portfolio/ > > > change your personal opinion? > > > > According to SFC's legal analysis, Microsoft joining the OIN > > doesn't mean that the eXFAT patents are covered, unless *Microsoft* > > contributes the code to the Linux usptream kernel. That's because > > the OIN is governed by the Linux System Definition, and until MS > > contributes code which covered by the exFAT patents, it doesn't > > count. > > > > For more details: > > > > https://sfconservancy.org/blog/2018/oct/10/microsoft-oin-exfat/ > > > > (This is not legal advice, and I am not a lawyer.) > > Interesting analysis. It seems to me that the correct forms would be > observed if someone suitably senior at Microsoft accepted the work > from Valdis and submitted it with their sign-off. KY, how about it? KY, if you need local help to convince anyone, I can do that ... I've been deeply involved in patent issues with open source from the community angle for a while and I'm used to talking to corporate counsels. Personally I think we could catch Microsoft in the implied licence to the FAT patent simply by putting exfat in the kernel and waiting for them to distribute it but I think it would benefit Microsoft much more from a community perspective to make an open donation of the FAT patents to Linux in much the same way they've already done for UEFI. If my analysis of the distribution situation is correct, it would be making a virtue of a necessity anyway which is always a useful business case argument. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: exfat filesystem
On Tue, 2019-07-09 at 12:37 -0400, Sasha Levin wrote: > On Tue, Jul 09, 2019 at 08:48:34AM -0700, Matthew Wilcox wrote: > > On Tue, Jul 09, 2019 at 11:30:39AM -0400, Theodore Ts'o wrote: > > > On Tue, Jul 09, 2019 at 04:21:36AM -0700, Matthew Wilcox wrote: > > > > How does > > > > https://www.zdnet.com/article/microsoft-open-sources-its-entire > > > > -patent-portfolio/ > > > > change your personal opinion? > > > > > > According to SFC's legal analysis, Microsoft joining the OIN > > > doesn't mean that the eXFAT patents are covered, unless > > > *Microsoft* contributes the code to the Linux usptream > > > kernel. That's because the OIN is governed by the Linux System > > > Definition, and until MS contributes code which covered by the > > > exFAT patents, it doesn't count. > > > > > > For more details: > > > > > > https://sfconservancy.org/blog/2018/oct/10/microsoft-oin-exfat/ > > > > > > (This is not legal advice, and I am not a lawyer.) > > > > Interesting analysis. It seems to me that the correct forms would > > be observed if someone suitably senior at Microsoft accepted the > > work from Valdis and submitted it with their sign-off. KY, how > > about it? > > Huh, that's really how this works? Let me talk with our lawyers to > clear this up. Not exactly, no. A corporate signoff is useful evidence of intent to bind patents, but a formal statement would be better and wouldn't require a signoff. The SFC analysis is also a bit lacking: hypothetically if exfat became part of Linux, it would be covered by the OIN legal definition which would place MS in an untenable position with regard to the mutual defence pact if it still wanted to enforce FAT patents against Linux. > Would this mean, hypothetically, that if MS has claims against the > kernel's scheduler for example, it can still assert them if no one > from MS touched the code? And then they lose that ability if a MS > employee adds a tiny fix in? No. You're already shipping a linux kernel, that makes Microsoft a distributor meaning you're bound by the GPL express patent licences so any patent Microsoft has on technology in the Linux kernel would be unenforceable under that. Plus as a member of OIN, you've guaranteed not to sue for any patent that reads on the Linux System definition, which is also a promise you can be held to. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 08/29] drivers, md: convert mddev.active from atomic_t to refcount_t
On Tue, 2017-03-14 at 12:29 +, Reshetova, Elena wrote: > > Elena Reshetova writes: > > > > > refcount_t type and corresponding API should be > > > used instead of atomic_t when the variable is used as > > > a reference counter. This allows to avoid accidental > > > refcounter overflows that might lead to use-after-free > > > situations. > > > > > > Signed-off-by: Elena Reshetova > > > Signed-off-by: Hans Liljestrand > > > Signed-off-by: Kees Cook > > > Signed-off-by: David Windsor > > > --- > > > drivers/md/md.c | 6 +++--- > > > drivers/md/md.h | 3 ++- > > > 2 files changed, 5 insertions(+), 4 deletions(-) > > > > When booting linux-next (specifically 5be4921c9958ec) I'm seeing > > the > > backtrace below. I suspect this patch is just exposing an existing > > issue? > > Yes, we have actually been following this issue in the another > thread. > It looks like the object is re-used somehow, but I can't quite > understand how just by reading the code. > This was what I put into the previous thread: > > "The log below indicates that you are using your refcounter in a bit > weird way in mddev_find(). > However, I can't find the place (just by reading the code) where you > would increment refcounter from zero (vs. setting it to one). > It looks like you either iterate over existing nodes (and increment > their counters, which should be >= 1 at the time of increment) or > create a new node, but then mddev_init() sets the counter to 1. " > > If you can help to understand what is going on with the object > creation/destruction, would be appreciated! > > Also Shaohua Li stopped this patch coming from his tree since the > issue was caught at that time, so we are not going to merge this > until we figure it out. Asking on the correct list (dm-devel) would have got you the easy answer: The refcount behind mddev->active is a genuine atomic. It has refcount properties but only if the array fails to initialise (in that case, final put kills it). Once it's added to the system as a gendisk, it cannot be freed until md_free(). Thus its ->active count can go to zero (when it becomes inactive; usually because of an unmount). On a simple allocation regardless of outcome, the last executed statement in md_alloc is mddev_put(): that destroys the device if we didn't manage to create it or returns 0 and adds an inactive device to the system which the user can get with mddev_find(). James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 0/7] scsi: storvsc: Miscellaneous enhancements and fixes
On Fri, 2015-03-27 at 00:26 -0700, K. Y. Srinivasan wrote: > This patch-set addresses perf issues discovered on the Azure storage stack. > These patches also fix a couple of bugs. > > As in the first version of this patch-set, some of the patches are simply a > resend. > I have bumped up the version number of all patches though. In this version, I > have > addressed issues raised by Olaf Hering , > Long Li and Venkatesh Srinivas Well, I was waiting for a reviewed-by from Olaf and Venkatesh, but I guess that's not forthcoming. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
On Fri, 2015-12-18 at 16:20 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: Hannes Reinecke [mailto:h...@suse.de]
> > Sent: Friday, December 18, 2015 12:52 AM
> > To: KY Srinivasan ; gre...@linuxfoundation.org;
> > linux-
> > ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > oher...@suse.com;
> > jbottom...@parallels.com; h...@infradead.org;
> > linux-s...@vger.kernel.org;
> > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > martin.peter...@oracle.com
> > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt
> > path
> >
> > On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > > On the interrupt path, we repeatedly establish the pointer to the
> > > storvsc_device. Fix this.
> > >
> > > Signed-off-by: K. Y. Srinivasan
> > > Reviewed-by: Long Li
> > > Reviewed-by: Johannes Thumshirn
> > > Tested-by: Alex Ng
> > > ---
> > > drivers/scsi/storvsc_drv.c | 23 ---
> > > 1 files changed, 8 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c
> > > b/drivers/scsi/storvsc_drv.c
> > > index d6ca4f2..b68aebe 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct
> > vmscsi_request *vm_srb,
> > > }
> > >
> > >
> > > -static void storvsc_command_completion(struct
> > > storvsc_cmd_request
> > *cmd_request)
> > > +static void storvsc_command_completion(struct
> > > storvsc_cmd_request
> > *cmd_request,
> > > +struct storvsc_device
> > > *stor_dev)
> > > {
> > > struct scsi_cmnd *scmnd = cmd_request->cmd;
> > > - struct hv_host_device *host_dev = shost_priv(scmnd
> > > ->device-
> > > host);
> > > struct scsi_sense_hdr sense_hdr;
> > > struct vmscsi_request *vm_srb;
> > > struct Scsi_Host *host;
> > > - struct storvsc_device *stor_dev;
> > > - struct hv_device *dev = host_dev->dev;
> > > u32 payload_sz = cmd_request->payload_sz;
> > > void *payload = cmd_request->payload;
> > >
> > > - stor_dev = get_in_stor_device(dev);
> > > host = stor_dev->host;
> > >
> > > vm_srb = &cmd_request->vstor_packet.vm_srb;
> > > @@ -987,14 +984,13 @@ static void
> > > storvsc_command_completion(struct
> > storvsc_cmd_request *cmd_request)
> > > kfree(payload);
> > > }
> > >
> > > -static void storvsc_on_io_completion(struct hv_device *device,
> > > +static void storvsc_on_io_completion(struct storvsc_device
> > > *stor_device,
> > > struct vstor_packet
> > > *vstor_packet,
> > > struct storvsc_cmd_request
> > > *request)
> > > {
> > > - struct storvsc_device *stor_device;
> > > struct vstor_packet *stor_pkt;
> > > + struct hv_device *device = stor_device->device;
> > >
> > > - stor_device = hv_get_drvdata(device);
> > > stor_pkt = &request->vstor_packet;
> > >
> > > /*
> > > @@ -1049,7 +1045,7 @@ static void storvsc_on_io_completion(struct
> > hv_device *device,
> > > stor_pkt->vm_srb.data_transfer_length =
> > > vstor_packet->vm_srb.data_transfer_length;
> > >
> > > - storvsc_command_completion(request);
> > > + storvsc_command_completion(request, stor_device);
> > >
> > > if (atomic_dec_and_test(&stor_device
> > > ->num_outstanding_req) &&
> > > stor_device->drain_notify)
> > > @@ -1058,21 +1054,19 @@ static void
> > > storvsc_on_io_completion(struct
> > hv_device *device,
> > >
> > > }
> > >
> > > -static void storvsc_on_receive(struct hv_device *device,
> > > +static void storvsc_on_receive(struct storvsc_device
> > > *stor_device,
> > >struct vstor_packet *vstor_packet,
> > >struct storvsc_cmd_request
> > > *request)
> > > {
> > > struct storvsc_scan_work *work;
> > > - struct storvsc_device *stor_device;
> > >
> > > switch (vstor_packet->operation) {
> > > case VSTOR_OPERATION_COMPLETE_IO:
> > > - storvsc_on_io_completion(device, vstor_packet,
> > > request);
> > > + storvsc_on_io_completion(stor_device,
> > > vstor_packet,
> > request);
> > > break;
> > >
> > > case VSTOR_OPERATION_REMOVE_DEVICE:
> > > case VSTOR_OPERATION_ENUMERATE_BUS:
> > > - stor_device = get_in_stor_device(device);
> > > work = kmalloc(sizeof(struct
> > > storvsc_scan_work),
> > GFP_ATOMIC);
> > > if (!work)
> > > return;
> > > @@ -1083,7 +1077,6 @@ static void storvsc_on_receive(struct
> > > hv_device
> > *device,
> > > break;
> > >
> > > case VSTOR_OPERATION_FCHBA_DATA:
> > > - stor_device = get_in_stor_device(device);
> > > cache_wwn(stor_device, vstor_packet);
> > > #i
Re: [PATCH V3 2/4] scsi: storvsc: Properly support Fibre Channel devices
On Fri, 2015-12-18 at 09:49 +0100, Hannes Reinecke wrote: > What I would like to see is a clear separation here: > - Disable FC disk handling if FC attributes are not configured > - Add a module parameter allowing to disable FC attributes even if > they are compiled in. Remember: this is a virtualized guest, and > people might want so save kernel memory wherever they can. So always > attaching to the fc transport template will make them very unhappy. > Alternatively you could split out FC device handling into a separate > driver, but seeing the diff that's probably overkill. I don't quite see how this can be a module parameter: the fc_transport_class is pulled in by symbol references. They won't go away whether a module parameter is zero or one. The only way to get the module not to link with a transport class is to have it not use the symbols at compile time (either because they're surrounded by an #ifdef or with an if() which the compiler evaluates at compile time to zero). In userspace you get around this with introspection and dlopen, but I don't think we have that functionality in the kernel. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt path
On Sat, 2015-12-19 at 02:28 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com
> > ]
> > Sent: Friday, December 18, 2015 8:48 AM
> > To: KY Srinivasan ; Hannes Reinecke <
> > h...@suse.de>;
> > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com;
> > jbottom...@parallels.com; h...@infradead.org;
> > linux-s...@vger.kernel.org;
> > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > martin.peter...@oracle.com
> > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the interrupt
> > path
> >
> > On Fri, 2015-12-18 at 16:20 +, KY Srinivasan wrote:
> > >
> > > > -Original Message-
> > > > From: Hannes Reinecke [mailto:h...@suse.de]
> > > > Sent: Friday, December 18, 2015 12:52 AM
> > > > To: KY Srinivasan ;
> > > > gre...@linuxfoundation.org;
> > > > linux-
> > > > ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > > > oher...@suse.com;
> > > > jbottom...@parallels.com; h...@infradead.org;
> > > > linux-s...@vger.kernel.org;
> > > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > > > martin.peter...@oracle.com
> > > > Subject: Re: [PATCH V3 4/4] scsi: storvsc: Tighten up the
> > > > interrupt
> > > > path
> > > >
> > > > On 12/13/2015 09:28 PM, K. Y. Srinivasan wrote:
> > > > > On the interrupt path, we repeatedly establish the pointer to
> > > > > the
> > > > > storvsc_device. Fix this.
> > > > >
> > > > > Signed-off-by: K. Y. Srinivasan
> > > > > Reviewed-by: Long Li
> > > > > Reviewed-by: Johannes Thumshirn
> > > > > Tested-by: Alex Ng
> > > > > ---
> > > > > drivers/scsi/storvsc_drv.c | 23 ---
> > > > > 1 files changed, 8 insertions(+), 15 deletions(-)
> > > > >
> > > > > diff --git a/drivers/scsi/storvsc_drv.c
> > > > > b/drivers/scsi/storvsc_drv.c
> > > > > index d6ca4f2..b68aebe 100644
> > > > > --- a/drivers/scsi/storvsc_drv.c
> > > > > +++ b/drivers/scsi/storvsc_drv.c
> > > > > @@ -945,19 +945,16 @@ static void storvsc_handle_error(struct
> > > > vmscsi_request *vm_srb,
> > > > > }
> > > > >
> > > > >
> > > > > -static void storvsc_command_completion(struct
> > > > > storvsc_cmd_request
> > > > *cmd_request)
> > > > > +static void storvsc_command_completion(struct
> > > > > storvsc_cmd_request
> > > > *cmd_request,
> > > > > +struct storvsc_device
> > > > > *stor_dev)
> > > > > {
> > > > > struct scsi_cmnd *scmnd = cmd_request->cmd;
> > > > > - struct hv_host_device *host_dev = shost_priv(scmnd
> > > > > ->device-
> > > > > host);
> > > > > struct scsi_sense_hdr sense_hdr;
> > > > > struct vmscsi_request *vm_srb;
> > > > > struct Scsi_Host *host;
> > > > > - struct storvsc_device *stor_dev;
> > > > > - struct hv_device *dev = host_dev->dev;
> > > > > u32 payload_sz = cmd_request->payload_sz;
> > > > > void *payload = cmd_request->payload;
> > > > >
> > > > > - stor_dev = get_in_stor_device(dev);
> > > > > host = stor_dev->host;
> > > > >
> > > > > vm_srb = &cmd_request->vstor_packet.vm_srb;
> > > > > @@ -987,14 +984,13 @@ static void
> > > > > storvsc_command_completion(struct
> > > > storvsc_cmd_request *cmd_request)
> > > > > kfree(payload);
> > > > > }
> > > > >
> > > > > -static void storvsc_on_io_completion(struct hv_device
> > > > > *device,
> > > > > +static void storvsc_on_io_completion(struct storvsc_device
> > > > > *stor_device,
> > > > > struct vstor_packet
> > > > > *vstor_packet,
> > > > > struct
> > > > > storvsc_cmd_r
Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote:
> The macro startget_to_rport() can return NULL; handle that case
> properly.
OK, can we unwind why you think you could possibly need this? It would
mean that fc_timed_out was called for a non-FC device, which was
thought to be an impossibility when the fc transport class was
designed.
James
> Signed-off-by: K. Y. Srinivasan
> Cc:
> ---
> drivers/scsi/scsi_transport_fc.c |2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/scsi_transport_fc.c
> b/drivers/scsi/scsi_transport_fc.c
> index 24eaaf6..42a908f 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -2081,7 +2081,7 @@ fc_timed_out(struct scsi_cmnd *scmd)
> {
> struct fc_rport *rport = starget_to_rport(scsi_target(scmd
> ->device));
>
> - if (rport->port_state == FC_PORTSTATE_BLOCKED)
> + if ((rport == NULL) || (rport->port_state ==
> FC_PORTSTATE_BLOCKED))
> return BLK_EH_RESET_TIMER;
>
> return BLK_EH_NOT_HANDLED;
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
On Fri, 2016-01-08 at 18:58 +, KY Srinivasan wrote: > > > -Original Message- > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com > > ] > > Sent: Thursday, January 7, 2016 3:49 PM > > To: KY Srinivasan ; gre...@linuxfoundation.org; > > linux- > > ker...@vger.kernel.org; de...@linuxdriverproject.org; > > oher...@suse.com; > > jbottom...@parallels.com; h...@infradead.org; > > linux-s...@vger.kernel.org; > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > > martin.peter...@oracle.com; h...@suse.de > > Cc: sta...@vger.kernel.org > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the > > error > > handling function > > > > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote: > > > The macro startget_to_rport() can return NULL; handle that case > > > properly. > > > > OK, can we unwind why you think you could possibly need this? It > > would > > mean that fc_timed_out was called for a non-FC device, which was > > thought to be an impossibility when the fc transport class was > > designed. > > As you know, on Hyper-V, FC devices are handled exactly like normal > scsi devices and the only additional information that is provided for > FC devices is the WWN for port and node. Till recently, I was not > publishing the WWN in the guest and so I was not even using the FC > transport. Recently, I implemented support for publishing the WWN in > the guest and for that I am using the FC transport for FC hosts. When > an FC LUN is dynamically removed, sometimes I see the timeout occurri > ng and since there is no rport associated with these devices I am > hitting the issue this patch is addressing. I could have addressed > this problem by establishing a storvsc specific time out function > even for FC devices - the same timeout function that I currently use > for scsi devices - storvsc_eh_timed_out(). I chose to instead fix > the fc_timed_out() function since the code was not handling a > possible condition. OK, so the specific problem is that the device is partly torn down when the timeout fires? I'm having a hard time seeing how we get a null rport in that case. The starget_to_rport() can only return NULL if the parent isn't an rport ... that shouldn't depend on the state of the FC device because the parent is torn down after the child. In any case, returning BLK_EH_RESET_TIMER will cause all sorts of problems because it resets the timer to fire again for the device. What you want is something to return BLK_EH_HANDLED which will just complete the request ... probably at a generic level, since this doesn't sound to be specific to FC. Something like the below ... assuming the teardown issue is the real problem. James --- diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 984ddcb..3c514c6 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -273,6 +273,10 @@ enum blk_eh_timer_return scsi_times_out(struct request *req) enum blk_eh_timer_return rtn = BLK_EH_NOT_HANDLED; struct Scsi_Host *host = scmd->device->host; + /* timeout for an already dead device, just kill the request */ + if (scmd->device->sdev_state == SDEV_DEL) + return BLK_EH_HANDLED; + trace_scsi_dispatch_cmd_timeout(scmd); scsi_log_completion(scmd, TIMEOUT_ERROR); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the error handling function
On Fri, 2016-01-08 at 20:12 +, KY Srinivasan wrote: > > > -Original Message- > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com > > ] > > Sent: Friday, January 8, 2016 11:21 AM > > To: KY Srinivasan ; gre...@linuxfoundation.org; > > linux- > > ker...@vger.kernel.org; de...@linuxdriverproject.org; > > oher...@suse.com; > > jbottom...@parallels.com; h...@infradead.org; > > linux-s...@vger.kernel.org; > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > > martin.peter...@oracle.com; h...@suse.de > > Cc: sta...@vger.kernel.org > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in the > > error > > handling function > > > > On Fri, 2016-01-08 at 18:58 +, KY Srinivasan wrote: > > > > > > > -Original Message- > > > > From: James Bottomley > > [mailto:james.bottom...@hansenpartnership.com > > > > ] > > > > Sent: Thursday, January 7, 2016 3:49 PM > > > > To: KY Srinivasan ; > > > > gre...@linuxfoundation.org; > > > > linux- > > > > ker...@vger.kernel.org; de...@linuxdriverproject.org; > > > > oher...@suse.com; > > > > jbottom...@parallels.com; h...@infradead.org; > > > > linux-s...@vger.kernel.org; > > > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > > > > martin.peter...@oracle.com; h...@suse.de > > > > Cc: sta...@vger.kernel.org > > > > Subject: Re: [PATCH 1/1] scsi: scsi_transport_fc: Fix a bug in > > > > the > > > > error > > > > handling function > > > > > > > > On Thu, 2016-01-07 at 16:40 -0800, K. Y. Srinivasan wrote: > > > > > The macro startget_to_rport() can return NULL; handle that > > > > > case > > > > > properly. > > > > > > > > OK, can we unwind why you think you could possibly need this? > > > > It > > > > would > > > > mean that fc_timed_out was called for a non-FC device, which > > > > was > > > > thought to be an impossibility when the fc transport class was > > > > designed. > > > > > > As you know, on Hyper-V, FC devices are handled exactly like > > > normal > > > scsi devices and the only additional information that is provided > > > for > > > FC devices is the WWN for port and node. Till recently, I was not > > > publishing the WWN in the guest and so I was not even using the > > > FC > > > transport. Recently, I implemented support for publishing the WWN > > > in > > > the guest and for that I am using the FC transport for FC hosts. > > > When > > > an FC LUN is dynamically removed, sometimes I see the timeout > > > occurri > > > ng and since there is no rport associated with these devices I am > > > hitting the issue this patch is addressing. I could have > > > addressed > > > this problem by establishing a storvsc specific time out function > > > even for FC devices - the same timeout function that I currently > > > use > > > for scsi devices - storvsc_eh_timed_out(). I chose to instead > > > fix > > > the fc_timed_out() function since the code was not handling a > > > possible condition. > > > > OK, so the specific problem is that the device is partly torn down > > when > > the timeout fires? I'm having a hard time seeing how we get a null > > rport in that case. The starget_to_rport() can only return NULL if > > the > > parent isn't an rport ... that shouldn't depend on the state of the > > FC > > device because the parent is torn down after the child. > > In our case, the parent is not an rport since I don't invoke > fc_remote_port_add() and so I do get a NULL value from the > starget_to_rport(). OK, so it's nothing to do with teardown? I'm going to need the FC people to comment on this. The transport class was apparently designed to allow use without rports. However, there are several places where we assume rports are present: The times out and the port block interface ... I'm betting all current users are rport otherwise we would have spotted this problem sooner. > > In any case, returning BLK_EH_RESET_TIMER will cause all sorts of > > problems because it resets the timer to fire again for the device. > > What you want is something to return BLK_EH_HANDLED which will > > just > > complete the request ... probably at a generic level,
Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V
On Thu, 2016-03-17 at 00:01 +, KY Srinivasan wrote:
> The only attributes I would be interested are:
> 1) node name
> 2) port name
>
> Ideally, if this can show under /sys/class/fc_host/hostx/port_name
> and node_name,
> it will be ideal since all user scripts can work.
OK, like this?
>From 7af7c428e7e04ddcc87fda12d6571e3dff8ae024 Mon Sep 17 00:00:00 2001
From: James Bottomley
Date: Fri, 18 Mar 2016 15:35:45 -0700
Subject: scsi_transport_fc: introduce lightweight class for virtualization
systems
The FC transport class is very heavily tilted towards helping things
which operate a fabric (as it should be). However, there seems to be
a need for a lightweight version for use in virtual systems that
simply want to show pass through FC information without making any use
of the heavyweight functions. This is an attempt to give them what
they want: the lightweight class has no vports or rports and only two
host attributes. Essentially, it's designed for the HV storvsc
driver, but if other virtualizataion systems have similar problems, we
can add more attributes.
Signed-off-by: James Bottomley
---
drivers/scsi/scsi_transport_fc.c | 94
include/scsi/scsi_transport_fc.h | 3 ++
2 files changed, 97 insertions(+)
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 8a88226..a9fcb4d 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -351,6 +351,27 @@ struct fc_internal {
#define to_fc_internal(tmpl) container_of(tmpl, struct fc_internal, t)
+#define FC_LW_HOST_NUM_ATTRS 2
+struct fc_lw_internal {
+ struct scsi_transport_template t;
+ struct fc_function_template *f;
+
+ /*
+* For attributes : each object has :
+* An array of the actual attributes structures
+* An array of null-terminated pointers to the attribute
+* structures - used for mid-layer interaction.
+*
+* The attribute containers for the starget and host are are
+* part of the midlayer. As the remote port is specific to the
+* fc transport, we must provide the attribute container.
+*/
+ struct device_attribute private_host_attrs[FC_LW_HOST_NUM_ATTRS];
+ struct device_attribute *host_attrs[FC_LW_HOST_NUM_ATTRS + 1];
+};
+
+#define to_fc_lw_internal(tmpl)container_of(tmpl, struct
fc_lw_internal, t)
+
static int fc_target_setup(struct transport_container *tc, struct device *dev,
struct device *cdev)
{
@@ -472,6 +493,12 @@ static int fc_host_remove(struct transport_container *tc,
struct device *dev,
return 0;
}
+static DECLARE_TRANSPORT_CLASS(fc_lw_host_class,
+ "fc_host",
+ NULL,
+ NULL,
+ NULL);
+
static DECLARE_TRANSPORT_CLASS(fc_host_class,
"fc_host",
fc_host_setup,
@@ -1968,6 +1995,25 @@ static int fc_host_match(struct attribute_container
*cont,
return &i->t.host_attrs.ac == cont;
}
+static int fc_lw_host_match(struct attribute_container *cont,
+ struct device *dev)
+{
+ struct Scsi_Host *shost;
+ struct fc_lw_internal *i;
+
+ if (!scsi_is_host_device(dev))
+ return 0;
+
+ shost = dev_to_shost(dev);
+ if (!shost->transportt || shost->transportt->host_attrs.ac.class
+ != &fc_lw_host_class.class)
+ return 0;
+
+ i = to_fc_lw_internal(shost->transportt);
+
+ return &i->t.host_attrs.ac == cont;
+}
+
static int fc_target_match(struct attribute_container *cont,
struct device *dev)
{
@@ -2171,6 +2217,54 @@ static int fc_it_nexus_response(struct Scsi_Host *shost,
u64 nexus, int result)
return i->f->it_nexus_response(shost, nexus, result);
}
+/**
+ * fc_attach_lw_transport - light weight attach function
+ * @ft:function template for optional attributes
+ *
+ * This attach function is to be used only for virtual FC emulators
+ * which do not have a physical fabric underneath them and thus only
+ * need a few attributes and no helper functions
+ */
+struct scsi_transport_template *
+fc_lw_attach_transport(struct fc_function_template *ft)
+{
+ int count;
+ struct fc_lw_internal *i = kzalloc(sizeof(struct fc_lw_internal),
+ GFP_KERNEL);
+
+ if (unlikely(!i))
+ return NULL;
+
+ i->t.host_attrs.ac.attrs = &i->host_attrs[0];
+ i->t.host_attrs.ac.class = &fc_lw_host_class.class;
+ i->t.host_attrs.ac.match = fc_lw_host_match;
+ i->t.host_size = sizeof(struct fc_host_attrs);
+ transport_container_reg
[PATCH 2/2] storvsc_drv: make use of the lightweight FC transport class
Signed-off-by: James Bottomley
---
drivers/scsi/storvsc_drv.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 3ddcabb..dcb7393 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1769,21 +1769,16 @@ static int __init storvsc_drv_init(void)
sizeof(u64)));
#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
- fc_transport_template = fc_attach_transport(&fc_transport_functions);
+ fc_transport_template = fc_lw_attach_transport(&fc_transport_functions);
if (!fc_transport_template)
return -ENODEV;
-
- /*
-* Install Hyper-V specific timeout handler.
-*/
- fc_transport_template->eh_timed_out = storvsc_eh_timed_out;
#endif
ret = vmbus_driver_register(&storvsc_drv);
#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
if (ret)
- fc_release_transport(fc_transport_template);
+ fc_lw_release_transport(fc_transport_template);
#endif
return ret;
@@ -1793,7 +1788,7 @@ static void __exit storvsc_drv_exit(void)
{
vmbus_driver_unregister(&storvsc_drv);
#if IS_ENABLED(CONFIG_SCSI_FC_ATTRS)
- fc_release_transport(fc_transport_template);
+ fc_lw_release_transport(fc_transport_template);
#endif
}
--
2.6.2
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V
On Wed, 2016-03-16 at 23:15 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com
> > ]
> > Sent: Wednesday, March 16, 2016 4:08 PM
> > To: Martin K. Petersen ; KY Srinivasan
> >
> > Cc: Christoph Hellwig ;
> > gre...@linuxfoundation.org;
> > linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > oher...@suse.com; jbottom...@parallels.com;
> > linux-s...@vger.kernel.org;
> > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com;
> > h...@suse.de
> > Subject: Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC
> > hosts on
> > Hyper-V
> >
> > On Wed, 2016-03-16 at 18:34 -0400, Martin K. Petersen wrote:
> > > > > > > > "KY" == KY Srinivasan writes:
> > >
> > > KY> How would I get the sysfs files under fc_host if I don't use
> > > the
> > > FC
> > > KY> transport. The customer scripts expect these sysfs files.
> > >
> > > Right, but I was interested in finding out why they need those
> > > files. And whether an alternative to the FC transport would be a
> > > better solution.
> >
> > If it's just the wwn file (or a set of other values), we might be
> > able
> > to separate that bit out of the FC transport class so you can use
> > it
> > independently ... do you have a full list of the files being used?
>
> Wwn files are what we can support on Hyper-V and that is what I want
> to support (to address customer requirements).
There is no wwn file. These are all the possible attributes they could
use; which one(s) do you want:
/*
* Setup SCSI Host Attributes.
*/
SETUP_HOST_ATTRIBUTE_RD(node_name);
SETUP_HOST_ATTRIBUTE_RD(port_name);
SETUP_HOST_ATTRIBUTE_RD(permanent_port_name);
SETUP_HOST_ATTRIBUTE_RD(supported_classes);
SETUP_HOST_ATTRIBUTE_RD(supported_fc4s);
SETUP_HOST_ATTRIBUTE_RD(supported_speeds);
SETUP_HOST_ATTRIBUTE_RD(maxframe_size);
if (ft->vport_create) {
SETUP_HOST_ATTRIBUTE_RD_NS(max_npiv_vports);
SETUP_HOST_ATTRIBUTE_RD_NS(npiv_vports_inuse);
}
SETUP_HOST_ATTRIBUTE_RD(serial_number);
SETUP_HOST_ATTRIBUTE_RD(manufacturer);
SETUP_HOST_ATTRIBUTE_RD(model);
SETUP_HOST_ATTRIBUTE_RD(model_description);
SETUP_HOST_ATTRIBUTE_RD(hardware_version);
SETUP_HOST_ATTRIBUTE_RD(driver_version);
SETUP_HOST_ATTRIBUTE_RD(firmware_version);
SETUP_HOST_ATTRIBUTE_RD(optionrom_version);
SETUP_HOST_ATTRIBUTE_RD(port_id);
SETUP_HOST_ATTRIBUTE_RD(port_type);
SETUP_HOST_ATTRIBUTE_RD(port_state);
SETUP_HOST_ATTRIBUTE_RD(active_fc4s);
SETUP_HOST_ATTRIBUTE_RD(speed);
SETUP_HOST_ATTRIBUTE_RD(fabric_name);
SETUP_HOST_ATTRIBUTE_RD(symbolic_name);
SETUP_HOST_ATTRIBUTE_RW(system_hostname);
/* Transport-managed attributes */
SETUP_PRIVATE_HOST_ATTRIBUTE_RW(dev_loss_tmo);
SETUP_PRIVATE_HOST_ATTRIBUTE_RW(tgtid_bind_type);
if (ft->issue_fc_host_lip)
SETUP_PRIVATE_HOST_ATTRIBUTE_RW(issue_lip);
if (ft->vport_create)
SETUP_PRIVATE_HOST_ATTRIBUTE_RW(vport_create);
if (ft->vport_delete)
SETUP_PRIVATE_HOST_ATTRIBUTE_RW(vport_delete);
/*
* Setup Remote Port Attributes.
*/
count=0;
SETUP_RPORT_ATTRIBUTE_RD(maxframe_size);
SETUP_RPORT_ATTRIBUTE_RD(supported_classes);
SETUP_RPORT_ATTRIBUTE_RW(dev_loss_tmo);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(node_name);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_name);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_id);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(roles);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(port_state);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RD(scsi_target_id);
SETUP_PRIVATE_RPORT_ATTRIBUTE_RW(fast_io_fail_tmo);
/*
* Setup Virtual Port Attributes.
*/
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(vport_state);
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(vport_last_state);
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(node_name);
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(port_name);
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(roles);
SETUP_PRIVATE_VPORT_ATTRIBUTE_RD(vport_type);
SETUP_VPORT_ATTRIBUTE_RW(symbolic_name);
SETUP_VPORT_ATTRIBUTE_WR(vport_delete);
SETUP_VPORT_ATTRIBUTE_WR(vport_disable);
I'm assuming it's host and rport port_id?
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Support manual scan of FC hosts on Hyper-V
On Wed, 2016-03-16 at 18:34 -0400, Martin K. Petersen wrote: > > > > > > "KY" == KY Srinivasan writes: > > KY> How would I get the sysfs files under fc_host if I don't use the > FC > KY> transport. The customer scripts expect these sysfs files. > > Right, but I was interested in finding out why they need those > files. And whether an alternative to the FC transport would be a > better solution. If it's just the wwn file (or a set of other values), we might be able to separate that bit out of the FC transport class so you can use it independently ... do you have a full list of the files being used? Thanks, James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot
On Wed, 2016-01-27 at 23:29 -0800, K. Y. Srinivasan wrote: > tree: https://na01.safelinks.protection.outlook.com/?url=https%3a%2 > f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2fli > nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad7108d3 > 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS%2ftO > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master > head: 03c21cb775a313f1ff19be59c5d02df3e3526471 > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc: Properly > support Fibre Channel devices > date: 3 weeks ago > config: x86_64-randconfig-s3-01281016 (attached as .config) > reproduce: > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9 > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > >drivers/built-in.o: In function `storvsc_remove': > > > storvsc_drv.c:(.text+0x213af7): undefined reference to > > > `fc_remove_host' >drivers/built-in.o: In function `storvsc_drv_init': > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to > > > `fc_attach_transport' > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to > > > `fc_release_transport' >drivers/built-in.o: In function `storvsc_drv_exit': > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to > > > `fc_release_transport' > > With this commit, the storvsc driver depends on FC atttributes. Make > this > dependency explicit. > > Signed-off-by: K. Y. Srinivasan > Reported-by: Fengguang Wu > --- > drivers/scsi/Kconfig |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index 64eed87..24365c3 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND > config HYPERV_STORAGE > tristate "Microsoft Hyper-V virtual storage driver" > depends on SCSI && HYPERV > + depends on SCSI_FC_ATTRS > default HYPERV > help > Select this option to enable the Hyper-V virtual storage > driver. OK, so I thought Hannes requested that you not make the hyperv driver depend on the FC attrs and you said you would ... has this changed? James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot
On Thu, 2016-01-28 at 19:07 +, KY Srinivasan wrote: > > > -Original Message- > > From: Olaf Hering [mailto:o...@aepfle.de] > > Sent: Thursday, January 28, 2016 7:56 AM > > To: KY Srinivasan > > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; oher...@suse.com; > > jbottom...@parallels.com; h...@infradead.org; > > linux-s...@vger.kernel.org; > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > > martin.peter...@oracle.com; h...@suse.de > > Subject: Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported > > by kbuild test > > robot > > > > On Wed, Jan 27, K. Y. Srinivasan wrote: > > > > > + depends on SCSI_FC_ATTRS > > > > I think 'depends' instead of 'select' will cause HYPERV_STORAGE to > > disapepar during make oldconfig if SCSI_FC_ATTRS was not set > > before. > > Not sure what the policy of 'depends' vs. 'select' actually is. > > If > > SCSI_FC_ATTRS is supposed to be a library kind of thing then > > 'select' > > might be the correct way. > > The current build issue is because the Hyper-V storage is configured > to be built with > the kernel while the SCSI_FC_AATRS is configured as a module. This > patch fixes that issue. > I too am not sure what the policy of using "depends" vs " select" is. > James, what would be your recommendation here. Oh, you don't care if FC_ATTRS are enabled, but if they are you need to exclude the case where storvsc is built in but FC_ATTRS is a module? This is the accepted way of doing it: depends on m || SCSI_FC_ATTRS != m James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V2 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot
On Fri, 2016-01-29 at 19:36 -0800, K. Y. Srinivasan wrote: > tree: https://na01.safelinks.protection.outlook.com/?url=https%3a%2 > f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2fli > nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad7108d3 > 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS%2ftO > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master > head: 03c21cb775a313f1ff19be59c5d02df3e3526471 > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc: Properly > support Fibre Channel devices > date: 3 weeks ago > config: x86_64-randconfig-s3-01281016 (attached as .config) > reproduce: > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9 > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > >drivers/built-in.o: In function `storvsc_remove': > > > storvsc_drv.c:(.text+0x213af7): undefined reference to > > > `fc_remove_host' >drivers/built-in.o: In function `storvsc_drv_init': > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to > > > `fc_attach_transport' > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to > > > `fc_release_transport' >drivers/built-in.o: In function `storvsc_drv_exit': > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to > > > `fc_release_transport' > > With this commit, the storvsc driver depends on FC atttributes. Make > this > dependency explicit. > > Signed-off-by: K. Y. Srinivasan > Reported-by: Fengguang Wu > --- > V2: Fixed the dependency based on suggestion by James Bottomley > > > drivers/scsi/Kconfig |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index 64eed87..ce0d07b 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND > config HYPERV_STORAGE > tristate "Microsoft Hyper-V virtual storage driver" > depends on SCSI && HYPERV > + depends on SCSI_FC_ATTRS || SCSI_FC_ATTRS !=m No ... if you want to depend on the FC_ATTRS then a simple depend works. If you want to be able to build without them or with them, then that line must read depends on m || SCSI_FC_ATTRS != m James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot
On Fri, 2016-02-26 at 15:45 -0800, K. Y. Srinivasan wrote: > tree: https://na01.safelinks.protection.outlook.com/?url=https%3a%2 > f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds%2fli > nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad7108d3 > 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS%2ftO > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master > head: 03c21cb775a313f1ff19be59c5d02df3e3526471 > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc: Properly > support Fibre Channel devices > date: 3 weeks ago > config: x86_64-randconfig-s3-01281016 (attached as .config) > reproduce: > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9 > # save the attached .config to linux build tree > make ARCH=x86_64 > > All errors (new ones prefixed by >>): > >drivers/built-in.o: In function `storvsc_remove': > > > storvsc_drv.c:(.text+0x213af7): undefined reference to > > > `fc_remove_host' >drivers/built-in.o: In function `storvsc_drv_init': > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to > > > `fc_attach_transport' > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to > > > `fc_release_transport' >drivers/built-in.o: In function `storvsc_drv_exit': > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to > > > `fc_release_transport' > > With this commit, the storvsc driver depends on FC atttributes. Make > this > dependency explicit. > > Signed-off-by: K. Y. Srinivasan > Reported-by: Fengguang Wu > --- > drivers/scsi/Kconfig |1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > index 64eed87..24365c3 100644 > --- a/drivers/scsi/Kconfig > +++ b/drivers/scsi/Kconfig > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND > config HYPERV_STORAGE > tristate "Microsoft Hyper-V virtual storage driver" > depends on SCSI && HYPERV > + depends on SCSI_FC_ATTRS Well, I suppose continually sending the wrong patch until I get annoyed enough to send the right one is one way of doing it. This patch is wrong. what you want is below. You want HYPERV_STORAGE to be built in if the FC attributes are, otherwise you don't care because if they're N the FC code will be compiled out. James --- diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig index e2f31c9..5ecabdb 100644 --- a/drivers/scsi/Kconfig +++ b/drivers/scsi/Kconfig @@ -596,6 +596,7 @@ config XEN_SCSI_FRONTEND config HYPERV_STORAGE tristate "Microsoft Hyper-V virtual storage driver" depends on SCSI && HYPERV + depends on m || SCSI_FC_ATTRS default HYPERV help Select this option to enable the Hyper-V virtual storage driver. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported by kbuild test robot
On Fri, 2016-02-26 at 23:22 +, KY Srinivasan wrote: > > > -Original Message- > > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com > > ] > > Sent: Friday, February 26, 2016 2:25 PM > > To: KY Srinivasan ; linux-ker...@vger.kernel.org > > ; > > de...@linuxdriverproject.org; oher...@suse.com; > > jbottom...@parallels.com; h...@infradead.org; > > linux-s...@vger.kernel.org; > > a...@canonical.com; vkuzn...@redhat.com; jasow...@redhat.com; > > martin.peter...@oracle.com; h...@suse.de > > Subject: Re: [PATCH 1/1] scsi: storvsc: Fix a build issue reported > > by kbuild test > > robot > > > > On Fri, 2016-02-26 at 15:45 -0800, K. Y. Srinivasan wrote: > > > tree: > > > https://na01.safelinks.protection.outlook.com/?url=https%3a%2 > > > f%2fgit.kernel.org%2fpub%2fscm%2flinux%2fkernel%2fgit%2ftorvalds% > > > 2fli > > > > > nux.git&data=01%7c01%7ckys%40microsoft.com%7ce2e0622715844b79ad71 > > 08d3 > > > > > 2796ec3c%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=ubr4GbBaNS > > %2ftO > > > z%2buJBk0CL9N0UNG9x2TidLgy6Yovg4%3d master > > > head: 03c21cb775a313f1ff19be59c5d02df3e3526471 > > > commit: dac582417bc449b1f7f572d3f1dd9d23eec15cc9 storvsc: > > > Properly > > > support Fibre Channel devices > > > date: 3 weeks ago > > > config: x86_64-randconfig-s3-01281016 (attached as .config) > > > reproduce: > > > git checkout dac582417bc449b1f7f572d3f1dd9d23eec15cc9 > > > # save the attached .config to linux build tree > > > make ARCH=x86_64 > > > > > > All errors (new ones prefixed by >>): > > > > > >drivers/built-in.o: In function `storvsc_remove': > > > > > storvsc_drv.c:(.text+0x213af7): undefined reference to > > > > > `fc_remove_host' > > >drivers/built-in.o: In function `storvsc_drv_init': > > > > > storvsc_drv.c:(.init.text+0xcbcc): undefined reference to > > > > > `fc_attach_transport' > > > > > storvsc_drv.c:(.init.text+0xcc06): undefined reference to > > > > > `fc_release_transport' > > >drivers/built-in.o: In function `storvsc_drv_exit': > > > > > storvsc_drv.c:(.exit.text+0x123c): undefined reference to > > > > > `fc_release_transport' > > > > > > With this commit, the storvsc driver depends on FC atttributes. > > > Make > > > this > > > dependency explicit. > > > > > > Signed-off-by: K. Y. Srinivasan > > > Reported-by: Fengguang Wu > > > --- > > > drivers/scsi/Kconfig |1 + > > > 1 files changed, 1 insertions(+), 0 deletions(-) > > > > > > diff --git a/drivers/scsi/Kconfig b/drivers/scsi/Kconfig > > > index 64eed87..24365c3 100644 > > > --- a/drivers/scsi/Kconfig > > > +++ b/drivers/scsi/Kconfig > > > @@ -594,6 +594,7 @@ config XEN_SCSI_FRONTEND > > > config HYPERV_STORAGE > > > tristate "Microsoft Hyper-V virtual storage driver" > > > depends on SCSI && HYPERV > > > + depends on SCSI_FC_ATTRS > > > > Well, I suppose continually sending the wrong patch until I get > > annoyed > > enough to send the right one is one way of doing it. This patch is > > wrong. what you want is below. > > James, that was not my intent; although it is a tempting strategy! > Here is an excerpt of your comments on v2 of this patch: > (dated Jan 29 of this year): > > "No ... if you want to depend on the FC_ATTRS then a simple depend > works. If you want to be able to build without them or with them, > then > that line must read > > depends on m || SCSI_FC_ATTRS != m" > > Since all I wanted was to depend on FC_ATTRS I chose to go with your > recommendation - > which also happened to be what I had initially sent (v1 of this > patch). If you're going to depend on the FC_ATTRS then you don't need all of the #if IS_ENABLED(CONFIG_SCSI_FC_ATTRS) In the driver, because you're never building without it. I thought those were put in to satisfy Hannes' request that the driver be buildable without the attributes? > > You want HYPERV_STORAGE to be built in if the FC attributes are, > > otherwise you don't care because if they're N the FC code will be > > compiled out. > > If FC attributes are built in, I have no issue - I want to be able to > build the storvsc driver both as a module as well as built in. > However, if storvsc is built as part of the kernel, I want to make > sure that FC attributes are also built as part of the kernel. The > build test failure that was reported was this case. With this patch, > the build issue reported by kbuild cannot happen since if the > FC_ATTRS are built as a module, storvsc cannot be builtin. Oh, right, the line should read depends on m || SCSI_FC_ATTRS != m for that case. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3] storvsc: use small sg_tablesize on x86
On Thu, 2016-06-09 at 18:25 +0200, Olaf Hering wrote:
> Reducing the sg_tablesize allows booting of 32bit kernels in VMs,
> after
> commit be0cf6ca301c61458dc4aa1a37acf4f58d2ed3d6 ("scsi: storvsc: Set
> the
> tablesize based on the information given by the host")
>
> [5.567138] hv_storvsc vmbus_1: adjusting sg_tablesize 0x800 ->
> 0x20
Are you really sure 32 is the correct size? That caps transfers at 128
k. Even in the dim, dark, pre-64bit (at least on x86) days, our static
limit for sg tablesize was 128 (0.5M).
I know 32 it what it had before, but the reason for the commit you
quote was to improve performance ...
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Drivers: scsi: FLUSH timeout
On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote: > > > -Original Message- > > From: geert.uytterhoe...@gmail.com [mailto:geert.uytterhoe...@gmail.com] > > On Behalf Of Geert Uytterhoeven > > Sent: Wednesday, September 25, 2013 1:40 AM > > To: KY Srinivasan > > Cc: Mike Christie; Jack Wang; Greg KH; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; > > h...@infradead.org; linux-s...@vger.kernel.org > > Subject: Re: Drivers: scsi: FLUSH timeout > > > > On Tue, Sep 24, 2013 at 11:53 PM, KY Srinivasan wrote: > > > I am not sure how that magic number was arrived at (the 60HZ number). We > > want this to be little higher - > > > > "60 * HZ" means "60 seconds". > > > > > would there be any issues raising this to say 180 seconds. This is the > > > value we > > currently have for I/O > > > timeout. > > > > So you want to replace it by "180 * HZ", which is ... another magic number? > > Ideally, I want this to be adjustable like the way we can change the I/O > timeout. > Since that has been attempted earlier and rejected (not clear what the > reasons were), > I was suggesting that we pick a larger number. James, let me know how I > should proceed here. It's only one problem device with its own driver, right? how about a per target value you set in target_configure? That way there's no damage to the rest of the system even in a mixed device environment. I don't see a particular need to expose this via sysfs unless someone else has a use case. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: Drivers: scsi: FLUSH timeout
On Fri, 2013-10-04 at 15:02 +, KY Srinivasan wrote: > > > -Original Message- > > From: Eric Seppanen [mailto:e...@purestorage.com] > > Sent: Thursday, October 03, 2013 1:49 PM > > To: Nicholas A. Bellinger > > Cc: KY Srinivasan; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; > > linux-s...@vger.kernel.org > > Subject: Re: Drivers: scsi: FLUSH timeout > > > > On Thu, Oct 3, 2013 at 5:09 AM, Nicholas A. Bellinger > > wrote: > > > > > > On Wed, 2013-10-02 at 18:29 +, KY Srinivasan wrote: > > > > Ideally, I want this to be adjustable like the way we can change the I/O > > timeout. > > > > Since that has been attempted earlier and rejected (not clear what the > > reasons were), > > > > I was suggesting that we pick a larger number. James, let me know how I > > should proceed here. > > > > > > > > > > I think the objection was to making a module parameter for doing this > > > globally for all struct scsi_disk, and not the idea of making it > > > adjustable on an individual basis per-say.. > > > > > > What about adding a /sys/class/scsi_disk/$HCTL/flush_timeout..? > > > > Do I/O timeouts and flush timeouts need to be independently adjusted? > > If you're having trouble with slow operations, it seems likely to be > > across the board. > > > > Flush timeout could be defined as 2x the read/write timeout. Any > > other command-specific timeouts could be scaled the same way. > > I like this idea and would result in minimal changes. James, if it ok with > you, > I could send you the patch. Depends: I still prefer the per-target override, but if the proposal is to take the existing variable timeout for the queue and have 2x that for the flush, so you plan to increase the per-device timeout with hyper-v to 90s via sysfs, then I'm OK with it. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> Rather than having a separate constant for specifying the timeout on FLUSH
> operations, use the basic I/O timeout value that is already configurable
> on a per target basis to derive the FLUSH timeout. Looking at the current
> definitions of these timeout values, the FLUSH operation is supposed to have
> a value that is twice the normal timeout value. This patch preserves this
> relationship while leveraging the flexibility of specifying the I/O timeout.
>
> I would like to thank Eric Seppanen and
> Nicholas A. Bellinger for their help in resolving
> this issue.
>
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/scsi/sd.c |5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e62d17d..8aff306 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
>
> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
> {
> - rq->timeout = SD_FLUSH_TIMEOUT;
> + rq->timeout *= 2;
> rq->retries = SD_MAX_RETRIES;
> rq->cmd[0] = SYNCHRONIZE_CACHE;
> rq->cmd_len = 10;
> @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> {
> int retries, res;
> struct scsi_device *sdp = sdkp->device;
> + unsigned int timeout = sdp->request_queue->rq_timeout;
The timeout is signed in the function prototype
> struct scsi_sense_hdr sshdr;
>
> if (!scsi_device_online(sdp))
> @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>* flush everything.
>*/
> res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> - &sshdr, SD_FLUSH_TIMEOUT,
> + &sshdr, timeout * 2,
>SD_MAX_RETRIES, NULL, REQ_PM);
> if (res == 0)
> break;
Not like this, please: you now leave us with a dangling #define whose
name makes you think it should be related to flushing and a couple of
curious magic constants. It's almost hand crafted to confuse people
reading the code.
Please do it like this instead: with a comment explaining what we're
doing above the #define and a name that clearly relates to the actions.
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index e62d17d..5c9496d 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
*sdp, struct request *rq)
static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
{
- rq->timeout = SD_FLUSH_TIMEOUT;
+ rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
rq->retries = SD_MAX_RETRIES;
rq->cmd[0] = SYNCHRONIZE_CACHE;
rq->cmd_len = 10;
@@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
{
int retries, res;
struct scsi_device *sdp = sdkp->device;
+ const int timeout = sdp->request_queue->rq_timeout
+ * SD_FLUSH_TIMEOUT_MULTIPLIER;
struct scsi_sense_hdr sshdr;
if (!scsi_device_online(sdp))
@@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
* flush everything.
*/
res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
-&sshdr, SD_FLUSH_TIMEOUT,
-SD_MAX_RETRIES, NULL, REQ_PM);
+&sshdr, timeout, SD_MAX_RETRIES,
+NULL, REQ_PM);
if (res == 0)
break;
}
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 7a049de..7f7999c 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -13,7 +13,11 @@
*/
#define SD_TIMEOUT (30 * HZ)
#define SD_MOD_TIMEOUT (75 * HZ)
-#define SD_FLUSH_TIMEOUT (60 * HZ)
+/*
+ * Flush timeout is a multiplier over the standard device timeout which is
+ * user modifiable via sysfs but initially set to SD_TIMEOUT
+ */
+#define SD_FLUSH_TIMEOUT_MULTIPLIER2
#define SD_WRITE_SAME_TIMEOUT (120 * HZ)
/*
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2013-10-04 at 22:01 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:jbottom...@parallels.com]
> > Sent: Friday, October 04, 2013 2:42 PM
> > To: KY Srinivasan
> > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org;
> > de...@linuxdriverproject.org; oher...@suse.com; h...@infradead.org;
> > e...@purestorage.com; n...@linux-iscsi.org; linux-s...@vger.kernel.org
> > Subject: Re: [PATCH 1/1] Drivers: scsi: Derive the FLUSH_TIMEOUT from the
> > basic
> > I/O timeout
> >
> > On Fri, 2013-10-04 at 12:38 -0700, K. Y. Srinivasan wrote:
> > > Rather than having a separate constant for specifying the timeout on FLUSH
> > > operations, use the basic I/O timeout value that is already configurable
> > > on a per target basis to derive the FLUSH timeout. Looking at the current
> > > definitions of these timeout values, the FLUSH operation is supposed to
> > > have
> > > a value that is twice the normal timeout value. This patch preserves this
> > > relationship while leveraging the flexibility of specifying the I/O
> > > timeout.
> > >
> > > I would like to thank Eric Seppanen and
> > > Nicholas A. Bellinger for their help in resolving
> > > this issue.
> > >
> > > Signed-off-by: K. Y. Srinivasan
> > > ---
> > > drivers/scsi/sd.c |5 +++--
> > > 1 files changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > > index e62d17d..8aff306 100644
> > > --- a/drivers/scsi/sd.c
> > > +++ b/drivers/scsi/sd.c
> > > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct
> > scsi_device *sdp, struct request *rq)
> > >
> > > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > > *rq)
> > > {
> > > - rq->timeout = SD_FLUSH_TIMEOUT;
> > > + rq->timeout *= 2;
> > > rq->retries = SD_MAX_RETRIES;
> > > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > > rq->cmd_len = 10;
> > > @@ -1433,6 +1433,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > > {
> > > int retries, res;
> > > struct scsi_device *sdp = sdkp->device;
> > > + unsigned int timeout = sdp->request_queue->rq_timeout;
> >
> > The timeout is signed in the function prototype
> >
> > > struct scsi_sense_hdr sshdr;
> > >
> > > if (!scsi_device_online(sdp))
> > > @@ -1448,7 +1449,7 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > >* flush everything.
> > >*/
> > > res = scsi_execute_req_flags(sdp, cmd, DMA_NONE, NULL, 0,
> > > - &sshdr, SD_FLUSH_TIMEOUT,
> > > + &sshdr, timeout * 2,
> > >SD_MAX_RETRIES, NULL, REQ_PM);
> > > if (res == 0)
> > > break;
> >
> > Not like this, please: you now leave us with a dangling #define whose
> > name makes you think it should be related to flushing and a couple of
> > curious magic constants. It's almost hand crafted to confuse people
> > reading the code.
> >
> > Please do it like this instead: with a comment explaining what we're
> > doing above the #define and a name that clearly relates to the actions.
> >
> > James
> >
> > ---
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index e62d17d..5c9496d 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -829,7 +829,7 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> > *sdp, struct request *rq)
> >
> > static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request
> > *rq)
> > {
> > - rq->timeout = SD_FLUSH_TIMEOUT;
> > + rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> > rq->retries = SD_MAX_RETRIES;
> > rq->cmd[0] = SYNCHRONIZE_CACHE;
> > rq->cmd_len = 10;
> > @@ -1433,6 +1433,8 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
> > {
> > int retries, res;
> > struct scsi_device *sdp = sdkp->device;
> > + const int timeout = sdp->request_queue->rq_timeout
> > + * SD_FLUSH_TIMEOUT_MULTIPLIER;
> > struct scsi_sense_hdr sshdr;
> >
> > if (!scsi_device_online(sdp))
> > @@ -1448,8 +1450,8 @@ static int sd_sync_cache(struct sc
Re: [PATCH 1/2] scsi: hyper-v storvsc switch up to SPC-3
On Fri, 2014-05-16 at 16:39 +0100, Ian Abbott wrote: > From: Andy Whitcroft > > Suggested-by: James Bottomley That is my patch, isn't it, just with a slightly modified comment: http://marc.info/?l=linux-scsi&m=137908428211951 Andy promised to go off and test it and that's where the thread ended. I take it the results of the testing was positive? I was expecting him to report back on that so KY could ack the patch. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] scsi: hyper-v storvsc -- host takes MAINTENANCE_IN commands badly -- elide them
On Fri, 2014-05-16 at 16:39 +0100, Ian Abbott wrote: > From: Andy Whitcroft > > BugLink: http://bugs.launchpad.net/bugs/1234417 This is a pretty low quality bug report; where's the analysis that should be in your patch? The problem is, is it not, that when you turn on trim we now also probe for write same using report opcodes (which is a SAI MAINTENANCE IN commad) and that's causing the boot hang. Firstly, that might be an indicator that the SPC3 route isn't a good one because there might be more hidden nasties like this ... and that goes back to how much testing have you done with the patch I suggested? If it is only a WRITE SAME problem, then the preferred route would be simply to set no_write_same in the template. James > Signed-off-by: Andy Whitcroft > Signed-off-by: Tim Gardner > --- > Original patch title was "UBUNTU: SAUCE: storvsc -- host takes > MAINTENANCE_IN commands badly elide them". Changed by Ian Abbott > > --- > drivers/scsi/storvsc_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index 3903c8a..c1eddd1 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c > @@ -1539,6 +1539,8 @@ static bool storvsc_scsi_cmd_ok(struct scsi_cmnd *scmnd) >* this. So, don't send it. >*/ > case SET_WINDOW: > + /* the host does not handle MAINTENANCE_IN preventing boot.*/ > + case MAINTENANCE_IN: > scmnd->result = ILLEGAL_REQUEST << 16; > allowed = false; > break; ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] scsi: hyper-v storvsc switch up to SPC-3
On Fri, 2014-05-16 at 18:39 +0100, Ian Abbott wrote: > On 2014-05-16 18:14, James Bottomley wrote: > > On Fri, 2014-05-16 at 16:39 +0100, Ian Abbott wrote: > >> From: Andy Whitcroft > >> > >> Suggested-by: James Bottomley > > > > That is my patch, isn't it, just with a slightly modified comment: > > > > http://marc.info/?l=linux-scsi&m=137908428211951 > > I believe so, yes. Looking at Ubuntu's kernel repository, Andy reverted > his original 4 patches and applied your patch instead. I'm not sure > about the other patch (PATCH 2/2) that disables the MAINTENANCE_IN > command. Perhaps that was needed as a consequence of claiming to be > SCSI level SPC-3? Yes, see other email. > > Andy promised to go off and test it and that's where the thread ended. I > > take it the results of the testing was positive? I was expecting him to > > report back on that so KY could ack the patch. > > > > James > > The patch seems to be in Ubuntu Saucy's 3.11 kernel version 3.11.0-12.18 > onwards - see > http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-saucy.git;a=log;h=refs/tags/Ubuntu-3.11.0-12.18 > > for the logs. > > Would you like me to resubmit the patch with you as the author? There > isn't a "Signed-off-by:" line for you on this patch at the moment. Is > it okay for me to add one? I'm not really comfortable with the way these patches are being submitted. I really need Andy to justify what's been done and why, then find an upstream acceptable format then for the Microsoft Hyper-V guys to ack them. We need more information than you can infer simply from the patches being in Ubuntu. If Andy's off somewhere, we can wait because this is just simply feature enablement; the bug doesn't show unless you enable trim on hv storvsc. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] scsi: hyper-v storvsc switch up to SPC-3
On Fri, 2014-05-16 at 19:18 +0100, Ian Abbott wrote: > On 2014-05-16 18:58, James Bottomley wrote: > > On Fri, 2014-05-16 at 18:39 +0100, Ian Abbott wrote: > >> On 2014-05-16 18:14, James Bottomley wrote: > >>> On Fri, 2014-05-16 at 16:39 +0100, Ian Abbott wrote: > >>>> From: Andy Whitcroft > >>>> > >>>> Suggested-by: James Bottomley > >>> > >>> That is my patch, isn't it, just with a slightly modified comment: > >>> > >>> http://marc.info/?l=linux-scsi&m=137908428211951 > >> > >> I believe so, yes. Looking at Ubuntu's kernel repository, Andy reverted > >> his original 4 patches and applied your patch instead. I'm not sure > >> about the other patch (PATCH 2/2) that disables the MAINTENANCE_IN > >> command. Perhaps that was needed as a consequence of claiming to be > >> SCSI level SPC-3? > > > > Yes, see other email. > > > >>> Andy promised to go off and test it and that's where the thread ended. I > >>> take it the results of the testing was positive? I was expecting him to > >>> report back on that so KY could ack the patch. > >>> > >>> James > >> > >> The patch seems to be in Ubuntu Saucy's 3.11 kernel version 3.11.0-12.18 > >> onwards - see > >> http://kernel.ubuntu.com/git?p=ubuntu/ubuntu-saucy.git;a=log;h=refs/tags/Ubuntu-3.11.0-12.18 > >> for the logs. > >> > >> Would you like me to resubmit the patch with you as the author? There > >> isn't a "Signed-off-by:" line for you on this patch at the moment. Is > >> it okay for me to add one? > > > > I'm not really comfortable with the way these patches are being > > submitted. I really need Andy to justify what's been done and why, then > > find an upstream acceptable format then for the Microsoft Hyper-V guys > > to ack them. We need more information than you can infer simply from > > the patches being in Ubuntu. If Andy's off somewhere, we can wait > > because this is just simply feature enablement; the bug doesn't show > > unless you enable trim on hv storvsc. > > TBH, I'm out of my depth on this, but hopefully I've kicked up the dust > a bit, since the patches (good or bad) have been languishing in Ubuntu's > repositories since October! That's OK, thanks for doing this; I'd forgotten about the patch. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/2] scsi: hyper-v storvsc changes by Ubuntu
On Thu, 2014-05-22 at 10:49 +, KY Srinivasan wrote: > > > > -Original Message- > > From: Andy Whitcroft [mailto:a...@canonical.com] > > Sent: Wednesday, May 21, 2014 7:25 AM > > To: Ian Abbott > > Cc: linux-s...@vger.kernel.org; de...@linuxdriverproject.org; KY Srinivasan; > > Haiyang Zhang; James E.J. Bottomley; Tim Gardner > > Subject: Re: [PATCH 0/2] scsi: hyper-v storvsc changes by Ubuntu > > > > On 16 May 2014 16:39, Ian Abbott wrote: > > > These changes to the Microsoft Hyper-V storage driver in Ubuntu > > > Saucy's > > > 3.13 kernel look useful for the mainline kernel, especially as they > > > enable 'TRIM' support. > > > > > > Andy Whitcroft (2): > > > scsi: hyper-v storvsc switch up to SPC-3 > > > scsi: hyper-v storvsc -- host takes MAINTENANCE_IN commands badly -- > > > elide them > > > > > > drivers/scsi/storvsc_drv.c | 10 ++ > > > 1 file changed, 10 insertions(+) > > > > > > The back story here is a little complex. The main issue is that the Hyper-V > > drives claim to be SPC-2, and yet implement the SPC-3 extensions for TRIM. > > We did attempt to upstream quirks to allow these features to be negotiated > > specifically for the Hyper-V virtual drives (minimum regression potential) > > but > > these were NAKd, and it was suggested that just overriding the Hyper-V > > drives to SPC-3 unconditionally was more appropriate. The first of the > > patches here does does this. This has been sitting in our tree for some > > time > > as it was not clear that this would be entirely safe, though the SPC-3 bits > > are > > in theory at least mostly detected. That said this change has been in > > Ubuntu > > for a full cycle now and does not seem to have caused any issues. If KY is > > happy we should likely submit it formally. The second fix I believed was > > already being submitted to mainline. > > > > KY? > > The Windows guys are not currently comfortable claiming conformance to > SPC-3, as they have not done > the necessary testing. This will change hopefully soon. Any bounds on the value of "soon" are we talking weeks or months? I think trim is a feature, which means no huge rush to get this in, but it is nice to respond in a timely fashion to a request from a distribution to enable a useful feature. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
> Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to derive the
> FLUSH_TIMEOUT from the basic I/O timeout. However, this patch did not use the
> basic I/O timeout of the device. Fix this bug.
>
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/scsi/sd.c |4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index e9689d5..54150b1 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct scsi_device
> *sdp, struct request *rq)
>
> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
> {
> - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> + int timeout = sdp->request_queue->rq_timeout;
> +
> + rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
Could you share where you found this to be a problem? It looks like a
bug in block because all inbound requests being prepared should have a
timeout set, so block would be the place to fix it.
I can't see how this can happen because we definitely add the timer
after the request is prepared in my reading of the block code, unless
I'm missing some path in block that violates this.
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2014-06-06 at 12:18 -0500, Mike Christie wrote:
> On 6/5/14, 9:53 PM, KY Srinivasan wrote:
> >
> >
> >> -Original Message-
> >> From: Mike Christie [mailto:micha...@cs.wisc.edu]
> >> Sent: Thursday, June 5, 2014 6:33 PM
> >> To: KY Srinivasan
> >> Cc: James Bottomley; linux-ker...@vger.kernel.org; a...@canonical.com;
> >> de...@linuxdriverproject.org; h...@infradead.org; linux-
> >> s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
> >> jasow...@redhat.com
> >> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT
> >> from the basic I/O timeout
> >>
> >> On 06/04/2014 12:15 PM, KY Srinivasan wrote:
> >>>
> >>>
> >>>> -Original Message-
> >>>> From: James Bottomley [mailto:jbottom...@parallels.com]
> >>>> Sent: Wednesday, June 4, 2014 10:02 AM
> >>>> To: KY Srinivasan
> >>>> Cc: linux-ker...@vger.kernel.org; a...@canonical.com;
> >>>> de...@linuxdriverproject.org; h...@infradead.org; linux-
> >>>> s...@vger.kernel.org; oher...@suse.com; gre...@linuxfoundation.org;
> >>>> jasow...@redhat.com
> >>>> Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the
> >>>> FLUSH_TIMEOUT from the basic I/O timeout
> >>>>
> >>>> On Wed, 2014-06-04 at 09:33 -0700, K. Y. Srinivasan wrote:
> >>>>> Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to
> >>>>> derive the FLUSH_TIMEOUT from the basic I/O timeout. However, this
> >>>>> patch did not use the basic I/O timeout of the device. Fix this bug.
> >>>>>
> >>>>> Signed-off-by: K. Y. Srinivasan
> >>>>> ---
> >>>>> drivers/scsi/sd.c |4 +++-
> >>>>> 1 files changed, 3 insertions(+), 1 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index
> >>>>> e9689d5..54150b1 100644
> >>>>> --- a/drivers/scsi/sd.c
> >>>>> +++ b/drivers/scsi/sd.c
> >>>>> @@ -832,7 +832,9 @@ static int sd_setup_write_same_cmnd(struct
> >>>>> scsi_device *sdp, struct request *rq)
> >>>>>
> >>>>> static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct
> >>>>> request *rq) {
> >>>>> - rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER;
> >>>>> + int timeout = sdp->request_queue->rq_timeout;
> >>>>> +
> >>>>> + rq->timeout = (timeout * SD_FLUSH_TIMEOUT_MULTIPLIER);
> >>>>
> >>>> Could you share where you found this to be a problem? It looks like
> >>>> a bug in block because all inbound requests being prepared should
> >>>> have a timeout set, so block would be the place to fix it.
> >>>
> >>> Perhaps; what I found was that the value in rq->timeout was 0 coming
> >>> into this function and thus multiplying obviously has no effect.
> >>>
> >>
> >> I think you are right. We hit this problem because we are doing:
> >>
> >> scsi_request_fn -> blk_peek_request -> sd_prep_fn ->
> >> scsi_setup_flush_cmnd.
> >>
> >> At this time request->timeout is zero so the multiplication does nothing.
> >> See
> >> how sd_setup_write_same_cmnd will set the request->timeout at this time.
> >>
> >> Then in scsi_request_fn we do:
> >>
> >> scsi_request_fn -> blk_start_request -> blk_add_timer.
> >>
> >> At this time it will set the request->timeout if something like req block
> >> pc
> >> users (like scsi_execute() or block/scsi_ioctl.c) or the write same code
> >> mentioned above have not set the timeout.
> >
> > I don't think this is a recent change. Prior to this commit, we were
> > setting the timeout
> > value in this function; it just happened to be a different constant
> > unrelated to the I/O
> > timeout.
> >
>
> Yeah, it looks like when 7e660100d85af860e7ad763202fff717adcdaacd was
> merged we were supposed to initialize it like in your patch in this thread.
>
> I guess we could do your patch in this thread, or if we want the block
> layer to initialize the timeout before the prep_fn callout is called
> then we would need to have the blk-flush.c code to that when it sets up
> the request. If we
Re: [PATCH 2/2] Drivers: scsi: storvsc: Force discovery of LUNs that may have been removed.
On Fri, 2014-08-29 at 10:13 +0200, Hannes Reinecke wrote: > On 08/29/2014 09:39 AM, Bart Van Assche wrote: > > On 08/29/14 08:19, Hannes Reinecke wrote: > >> On 08/29/2014 04:42 AM, Mike Christie wrote: > >>> How are distros handling 0x6/0x3f/0x0e (report luns changed) when it > >>> gets passed to userspace? Is everyone kicking off a new full (add and > >>> delete) scan to handle this or logging it? Is the driver returning this > >>> when the LUNs change? > >>> > >> Currently it's logged to userspace and ignored. > >> Doing an automated rescan has proven to be dangerous, as it > >> might disconnect any LUNs which are still in use by applications. > >> Especially HA or database setups tends to become very annoyed > >> when you do an automated rescan. > > > > Has it already been considered to add newly discovered LUNs > > automatically and to leave it to the user to remove stale LUNs manually > > ? That would be similar to what the rescan-scsi-bus.sh script does > > without option -r/--remove. > > > > As of now we're still missing an in-kernel infrastructure which > would allow us to react on any sense codes; currently we're relying > on the administrator to setup a udev rule here. Um, I thought this was supposed to solve that problem: commit 279afdfe78a020b4b1a68bffd0009b961b12982e Author: Ewan D. Milne Date: Thu Aug 8 15:07:48 2013 -0400 [SCSI] Generate uevents on certain unit attention codes The idea was supposed to be that, as you say, log scrubbers are hard to configure and break every time someone fixes a spelling error, so we could now listen for a report luns data change uevent instead. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/3] scsi: Add Hyper-V logical block provisioning quirks
On Sat, 2014-10-11 at 10:39 -0700, Christoph Hellwig wrote: > On Fri, Oct 10, 2014 at 08:49:01AM +0100, Sitsofe Wheeler wrote: > > Microsoft Hyper-V virtual disks currently only claim SPC-2 compliance > > even though they implement post SPC-2 features (such as thin > > provisioning) which means the Linux kernel does not go on to test for > > those features even though they are advertised. > > > > A previous patch attempted to add a quirk to workaround this but the > > quirk was only enabled after the features had been scanned for, wouldn't > > work for "small" disks and would quirk on all Hyper-V SCSI devices > > (e.g. passthrough disks). > > > > The new patches partially revert the previous effort, add the quirk in a > > more traditional manner to only Hyper-V virtual disks and work on small > > virtual disks. > > This seems like might want a quirk to simply "force" a SPC3 compliance > level? This was initially suggested, but rejected by Microsoft because of other problems advertising SPC-3 compliance brings. Perhaps the hyper-v emulator has matured sufficiently that it will now work OK? James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] Drivers: hv: vmbus: fix init_vp_index() for reloading hv_netvsc
On Thu, 2015-08-13 at 17:07 -0700, K. Y. Srinivasan wrote:
> From: Dexuan Cui
>
> This fixes the recent commit 3b71107d73b16074afa7658f3f0fcf837aabfe24:
Which tree is this in? upstream linus is giving me bad object on that
id.
> Drivers: hv: vmbus: Further improve CPU affiliation logic
>
> Without the fix, reloading hv_netvsc hangs the guest.
The reason for looking for the commit id was to see if cc to stable was
necessary, is it?
James
> Signed-off-by: Dexuan Cui
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/hv/channel_mgmt.c | 17 +
> 1 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 3ab4753..8a4105c 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -204,6 +204,8 @@ void hv_process_channel_removal(struct vmbus_channel
> *channel, u32 relid)
> spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
> list_del(&channel->listentry);
> spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
> +
> + primary_channel = channel;
> } else {
> primary_channel = channel->primary_channel;
> spin_lock_irqsave(&primary_channel->lock, flags);
> @@ -211,6 +213,14 @@ void hv_process_channel_removal(struct vmbus_channel
> *channel, u32 relid)
> primary_channel->num_sc--;
> spin_unlock_irqrestore(&primary_channel->lock, flags);
> }
> +
> + /*
> + * We need to free the bit for init_vp_index() to work in the case
> + * of sub-channel, when we reload drivers like hv_netvsc.
> + */
> + cpumask_clear_cpu(channel->target_cpu,
> + &primary_channel->alloced_cpus_in_node);
> +
> free_channel(channel);
> }
>
> @@ -457,6 +467,13 @@ static void init_vp_index(struct vmbus_channel *channel,
> const uuid_le *type_gui
> continue;
> }
>
> + /*
> + * NOTE: in the case of sub-channel, we clear the sub-channel
> + * related bit(s) in primary->alloced_cpus_in_node in
> + * hv_process_channel_removal(), so when we reload drivers
> + * like hv_netvsc in SMP guest, here we're able to re-allocate
> + * bit from primary->alloced_cpus_in_node.
> + */
> if (!cpumask_test_cpu(cur_cpu,
> &primary->alloced_cpus_in_node)) {
> cpumask_set_cpu(cur_cpu,
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH RESEND V2 3/7] scsi: storvsc: Untangle the storage protocol negotiation from the vmbus protocol negotiation.
On Thu, 2015-08-13 at 08:43 -0700, K. Y. Srinivasan wrote:
> From: Keith Mange
>
> Currently we are making decisions based on vmbus protocol versions
> that have been negotiated; use storage potocol versions instead.
>
> Tested-by: Alex Ng
> Signed-off-by: Keith Mange
> Signed-off-by: K. Y. Srinivasan
> ---
> drivers/scsi/storvsc_drv.c | 109
> +++-
> 1 files changed, 87 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 5f9d133..f29871e 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -56,14 +56,18 @@
> * V1 RC > 2008/1/31: 2.0
> * Win7: 4.2
> * Win8: 5.1
> + * Win8.1: 6.0
> + * Win10: 6.2
> */
>
> #define VMSTOR_PROTO_VERSION(MAJOR_, MINOR_) MAJOR_) & 0xff) << 8) | \
> (((MINOR_) & 0xff)))
>
> +#define VMSTOR_PROTO_VERSION_WIN6VMSTOR_PROTO_VERSION(2, 0)
> #define VMSTOR_PROTO_VERSION_WIN7VMSTOR_PROTO_VERSION(4, 2)
> #define VMSTOR_PROTO_VERSION_WIN8VMSTOR_PROTO_VERSION(5, 1)
> -
> +#define VMSTOR_PROTO_VERSION_WIN8_1 VMSTOR_PROTO_VERSION(6, 0)
> +#define VMSTOR_PROTO_VERSION_WIN10 VMSTOR_PROTO_VERSION(6, 2)
>
> /* Packet structure describing virtual storage requests. */
> enum vstor_packet_operation {
> @@ -205,6 +209,46 @@ struct vmscsi_request {
>
>
> /*
> + * The list of storage protocols in order of preference.
> + */
> +struct vmstor_protocol {
> + int protocol_version;
> + int sense_buffer_size;
> + int vmscsi_size_delta;
> +};
> +
> +#define VMSTOR_NUM_PROTOCOLS5
> +
> +const struct vmstor_protocol vmstor_protocols[VMSTOR_NUM_PROTOCOLS] = {
Sparse doesn't like this not being static:
CHECK drivers/scsi/storvsc_drv.c
drivers/scsi/storvsc_drv.c:221:30: warning: symbol 'vmstor_protocols'
was not declared. Should it be static?
I fixed it up.
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] storvsc: Don't set the SRB_FLAGS_QUEUE_ACTION_ENABLE flag
On Mon, 2015-08-31 at 08:21 -0700, K. Y. Srinivasan wrote:
> Don't set the SRB_FLAGS_QUEUE_ACTION_ENABLE flag since we are not specifying
> tags.
What's the actual problem description this causes?
James
> Signed-off-by: K. Y. Srinivasan
> Cc: sta...@vger.kernel.org
> ---
> drivers/scsi/storvsc_drv.c |3 +--
> 1 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 40c43ae..ad8c4bc 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1647,8 +1647,7 @@ static int storvsc_queuecommand(struct Scsi_Host *host,
> struct scsi_cmnd *scmnd)
> vm_srb->win8_extension.time_out_value = 60;
>
> vm_srb->win8_extension.srb_flags |=
> - (SRB_FLAGS_QUEUE_ACTION_ENABLE |
> - SRB_FLAGS_DISABLE_SYNCH_TRANSFER);
> + SRB_FLAGS_DISABLE_SYNCH_TRANSFER;
>
> /* Build the SRB */
> switch (scmnd->sc_data_direction) {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > -Original Message- > > From: Christoph Hellwig [mailto:h...@infradead.org] > > Sent: Wednesday, July 9, 2014 1:43 AM > > To: KY Srinivasan > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; > > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com; > > a...@canonical.com; linux-s...@vger.kernel.org; sta...@vger.kernel.org > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > Host does not handle WRITE_SAME_16; filter this command out. This > > > patch is required to handle large devices (greater than 2 TB disks). > > > > Storvsc already sets the no_write_same flag, where is the command coming > > from? > > In spite of this flag, I see WRITE_SAME_16 being issued when I format a > device bigger than 2 TB; > I tried both xfs and ext4. Windows hosts currently do not handle unsupported > commands correctly - > The information returned is not sufficient to effect recovery in the Linux > guest. While this may be addressed in > future hosts, this patch fixes the problem. What Christoph means is that this looks like a bug somewhere in SCSI itself. That means we need to find it and kill it, not add workarounds to every driver that sets no_write_same ... James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote:
>
> > -Original Message-
> > From: James Bottomley [mailto:jbottom...@parallels.com]
> > Sent: Wednesday, July 9, 2014 12:57 PM
> > To: KY Srinivasan
> > Cc: linux-ker...@vger.kernel.org; h...@infradead.org;
> > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org;
> > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com
> > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
> >
> > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote:
> > >
> > > > -Original Message-
> > > > From: Christoph Hellwig [mailto:h...@infradead.org]
> > > > Sent: Wednesday, July 9, 2014 1:43 AM
> > > > To: KY Srinivasan
> > > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org;
> > > > oher...@suse.com; jbottom...@parallels.com; jasow...@redhat.com;
> > > > a...@canonical.com; linux-s...@vger.kernel.org;
> > > > sta...@vger.kernel.org
> > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter
> > > > WRITE_SAME_16
> > > >
> > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote:
> > > > > Host does not handle WRITE_SAME_16; filter this command out. This
> > > > > patch is required to handle large devices (greater than 2 TB disks).
> > > >
> > > > Storvsc already sets the no_write_same flag, where is the command
> > > > coming from?
> > >
> > > In spite of this flag, I see WRITE_SAME_16 being issued when I format
> > > a device bigger than 2 TB; I tried both xfs and ext4. Windows hosts
> > > currently do not handle unsupported commands correctly - The
> > > information returned is not sufficient to effect recovery in the Linux
> > > guest.
> > While this may be addressed in future hosts, this patch fixes the problem.
> >
> > What Christoph means is that this looks like a bug somewhere in SCSI itself.
> > That means we need to find it and kill it, not add workarounds to every
> > driver
> > that sets no_write_same ...
>
> James,
>
> I will try to isolate this issue in the SCSI stack. If it is ok with you
> guys, I would still want to filter WRITE_SAME_16
> (as we currently do WRITE_SAME) in our driver since this would address the
> problem for a large number of
> customers on our platform.
If we fix it at source, why would there be any need to filter? That's
the reason the no_write_same flag was introduced. If we can find and
fix the bug, it can go back into the stable trees as a bug fix, hence
nothing should ever emit write_same(10 or 16) and additional driver code
is redundant (and counter productive, since if this ever breaks again
you're our best canary).
This looks like it might be the problem but Martin should confirm (I
think the problem comes to us from the RC16 code which unconditionally
sets WS16).
James
---
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6825eda..8353a4c 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -634,6 +634,23 @@ static void sd_config_discard(struct scsi_disk *sdkp,
unsigned int mode)
max(sdkp->physical_block_size,
sdkp->unmap_granularity * logical_block_size);
+ if (sdkp->device->host->no_write_same) {
+ switch(mode) {
+
+ case SD_LBP_WS16:
+ case SD_LBP_WS10:
+ case SD_LBP_ZERO:
+ /*
+* filter out all the WRITE SAME modes and map them
+* directly to UNMAP
+*/
+ mode = SD_LBP_UNMAP;
+ /* fall through */
+ default:
+ /* everything else is OK */
+ break;
+ }
+ }
sdkp->provisioning_mode = mode;
switch (mode) {
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Thu, 2014-07-10 at 21:02 +, KY Srinivasan wrote: > > > -Original Message- > > From: James Bottomley [mailto:jbottom...@parallels.com] > > Sent: Wednesday, July 9, 2014 3:27 PM > > To: KY Srinivasan > > Cc: linux-ker...@vger.kernel.org; m...@mkp.net; h...@infradead.org; > > de...@linuxdriverproject.org; a...@canonical.com; sta...@vger.kernel.org; > > linux-s...@vger.kernel.org; oher...@suse.com; jasow...@redhat.com > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16 > > > > On Wed, 2014-07-09 at 21:14 +, KY Srinivasan wrote: > > > > > > > -Original Message- > > > > From: James Bottomley [mailto:jbottom...@parallels.com] > > > > Sent: Wednesday, July 9, 2014 12:57 PM > > > > To: KY Srinivasan > > > > Cc: linux-ker...@vger.kernel.org; h...@infradead.org; > > > > de...@linuxdriverproject.org; a...@canonical.com; > > > > sta...@vger.kernel.org; linux-s...@vger.kernel.org; > > > > oher...@suse.com; jasow...@redhat.com > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > > WRITE_SAME_16 > > > > > > > > On Wed, 2014-07-09 at 19:52 +, KY Srinivasan wrote: > > > > > > > > > > > -Original Message- > > > > > > From: Christoph Hellwig [mailto:h...@infradead.org] > > > > > > Sent: Wednesday, July 9, 2014 1:43 AM > > > > > > To: KY Srinivasan > > > > > > Cc: linux-ker...@vger.kernel.org; de...@linuxdriverproject.org; > > > > > > oher...@suse.com; jbottom...@parallels.com; > > jasow...@redhat.com; > > > > > > a...@canonical.com; linux-s...@vger.kernel.org; > > > > > > sta...@vger.kernel.org > > > > > > Subject: Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter > > > > > > WRITE_SAME_16 > > > > > > > > > > > > On Tue, Jul 08, 2014 at 05:46:48PM -0700, K. Y. Srinivasan wrote: > > > > > > > Host does not handle WRITE_SAME_16; filter this command out. > > > > > > > This patch is required to handle large devices (greater than 2 TB > > disks). > > > > > > > > > > > > Storvsc already sets the no_write_same flag, where is the > > > > > > command coming from? > > > > > > > > > > In spite of this flag, I see WRITE_SAME_16 being issued when I > > > > > format a device bigger than 2 TB; I tried both xfs and ext4. > > > > > Windows hosts currently do not handle unsupported commands > > > > > correctly - The information returned is not sufficient to effect > > > > > recovery > > in the Linux guest. > > > > While this may be addressed in future hosts, this patch fixes the > > > > problem. > > > > > > > > What Christoph means is that this looks like a bug somewhere in SCSI > > itself. > > > > That means we need to find it and kill it, not add workarounds to > > > > every driver that sets no_write_same ... > > > > > > James, > > > > > > I will try to isolate this issue in the SCSI stack. If it is ok with > > > you guys, I would still want to filter WRITE_SAME_16 (as we currently > > > do WRITE_SAME) in our driver since this would address the problem for a > > large number of customers on our platform. > > > > If we fix it at source, why would there be any need to filter? That's the > > reason the no_write_same flag was introduced. If we can find and fix the > > bug, it can go back into the stable trees as a bug fix, hence nothing should > > ever emit write_same(10 or 16) and additional driver code is redundant (and > > counter productive, since if this ever breaks again you're our best canary). > > > > This looks like it might be the problem but Martin should confirm (I think > > the > > problem comes to us from the RC16 code which unconditionally sets WS16). > > > > James > > James, > > This patch works for me; are you planning on committing this patch. OK, if we go with this we can add your tested by. It's not going in until Martin takes a look because there may be a better way of doing this. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 04:01 -0700, h...@infradead.org wrote: > On Sun, Jul 13, 2014 at 08:58:34AM -0400, Martin K. Petersen wrote: > > > "KY" == KY Srinivasan writes: > > > > KY> Windows hosts do support UNMAP and set the field in the > > KY> EVPD. However, since the host advertises SPC-2 compliance, Linux > > KY> does not even query the VPD page. > > > > >> If we want to enable UNMAP in this case I'd prefer a blacklist entry > > >> than trying UNMAP despite the device not advertising it. > > > > I agree with that. We could do something like the patch below. > > > > However, I do think it's a good idea that you guys are looking into > > reporting SPC-3. > > KY mentioned that they have a prototype for that now. > > Btw, I looked over sd.c a bit more, and I think I understand why they > get the WRITE SAME commands now: > > read_capacity_16 calls sd_config_discard(sdkp, SD_LBP_WS16) if the LPBME > bit is set. At least older SBC drafts left it wide open if a target > supports WRITE SAME with UNMAP or UNMAP in this case. So I think we'd > still want a patch to use UNMAP instead of WRITE SAME for this case, > which should also fix hyperv. Below is the quick hack version of that > that just checks the host no_write_same flag, as the one on the device > isn't set yet - I guess we need to refactor some of that logic. > > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 87566b5..4480fdf 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2035,7 +2035,10 @@ static int read_capacity_16(struct scsi_disk *sdkp, > struct scsi_device *sdp, > if (buffer[14] & 0x40) /* LBPRZ */ > sdkp->lbprz = 1; > > - sd_config_discard(sdkp, SD_LBP_WS16); > + if (sdp->host->no_write_same) > + sd_config_discard(sdkp, SD_LBP_UNMAP); > + else > + sd_config_discard(sdkp, SD_LBP_WS16); Right, I already said this was the problem: that was the reason for my patch. However, there are a couple of other cases (including the /sys entry) which is why I patched sd_config_discard instead. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 13:47 -0400, Martin K. Petersen wrote: > > "Christoph" == hch@infradead org writes: > > Christoph> Oh, we actually have devices that support WRITE SAME with > Christoph> unmap, but not without? That's defintively a little strange. > > Yep :( > > There were several SSDs that did not want to support wearing out flash > by writing gobs of zeroes and only support the UNMAP case. > > Christoph> Yes, and it did this intentionally. I really wouldn't expect > Christoph> devices to support WRITE SAME with UNMAP but blow up on a > Christoph> WRITE SAME without it (and not just simple fail it in an > Christoph> orderly way). > > *sigh* > > Christoph> It definitively seems odd to default to trying WRITE SAME for > Christoph> unmap for a device that explicitly tells us that it doesn't > Christoph> support WRITE SAME. > > Maybe it's just a naming thing. I was really trying to convey > no_req_write_same support, not no_write_same_10_or_16. > > Christoph> Note that I'm not against your patch - I suspect forcing us > Christoph> to read EVPD pages even for devices that claim to be SPC-2 > Christoph> will come in useful in various scenarios. > > I don't have a problem with a BLIST_PREFER_UNMAP flag or something like > that. But BLIST_TRY_VPD_PAGES seems more generally useful and it does > fix the problem at hand. That's why I went that route. Hang on ... unless we apply Christoph or my fix, we'll get the same issue with every raid driver (that's about 10 I think) that set no_write_same when they hit a >2TB RAID volume, so I think we need both fixes. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 14:45 -0400, Martin K. Petersen wrote:
> >>>>> "James" == James Bottomley writes:
>
> >> I don't have a problem with a BLIST_PREFER_UNMAP flag or something
> >> like that. But BLIST_TRY_VPD_PAGES seems more generally useful and it
> >> does fix the problem at hand. That's why I went that route.
>
> James> Hang on ... unless we apply Christoph or my fix, we'll get the
> James> same issue with every raid driver (that's about 10 I think) that
> James> set no_write_same when they hit a >2TB RAID volume,
>
> no_write_same is set for all the RAID controller drivers (54b2b50c20a6).
>
> If a WRITE SAME(10/16) command fails and the UNMAP bit is not set we'll
> set no_write_same=1 and disable REQ_WRITE_SAME support.
>
> If a WRITE SAME(10/16) command fails and the UNMAP bit is set we'll
> disable REQ_DISCARD support.
>
> Not sure where the 10 vs. 16 byte 2TB limitation comes into play here?
It's the code we identified in sd.c:read_capacity_16():
if (buffer[14] & 0x80) { /* LBPME */
sdkp->lbpme = 1;
if (buffer[14] & 0x40) /* LBPRZ */
sdkp->lbprz = 1;
sd_config_discard(sdkp, SD_LBP_WS16);
}
If the inquiry shows LBPME set, we'll do write same regardless of the
no_write_same bit. That's why for SPC-2 devices it only shows up on
>2TB devices because they force RC16.
James
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 4/8] Drivers: scsi: storvsc: Filter WRITE_SAME_16
On Wed, 2014-07-16 at 15:08 -0400, Martin K. Petersen wrote: > >>>>> "James" == James Bottomley writes: > > James> It's the code we identified in sd.c:read_capacity_16(): > > That's there to support devices that implement thin provisioning but > which predate the LBP VPD page. And thus have no way to tell us their > preferred command variant. > > James> If the inquiry shows LBPME set, we'll do write same regardless of > James> the no_write_same bit. That's why for SPC-2 devices it only > James> shows up on >> 2TB devices because they force RC16. > > But we only do so if they have LBPME set. Generally they don't. > > The problem for hyperv was that LBPME was set. And because it claimed > SBC-2 we did not check the LBP VPD page to see that it prefers UNMAP. > > I believe we did consider unconditionally checking for VPDs when LBPME > was set but I seem to recall that it broke some device that had garbage > in the READ CAPACITY(16) response. I'll see if I can locate the details. > > Otherwise I'm willing to entertain that idea. Well, your judgement: is this situation (support for UNMAP but not for WRITE_SAME) in what is effectively a RAID driver (hv drivers count as RAID) just a silly Microsoft one off? If so, we can go with your force VPD page patch. However, if we get any RAID drivers with strange discard support, we'll likely get the same problem ... I've got to say in the long run, it sounds likely we will given that RAID vendors are only marginally more competent than USB bridge vendors when it comes to following the spec. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2014-07-18 at 00:51 +, Elliott, Robert (Server Storage) wrote: > In sd_sync_cache: > rq->timeout *= SD_FLUSH_TIMEOUT_MULTIPLIER; > > Regardless of the baseline for the multiplication, a magic > number of 2 is too arbitrary. That might work for an > individual drive, but could be far too short for a RAID > controller that runs into worst case error handling for > the drives to which it is flushing (e.g., if its cache > is volatile and the drives all have recoverable errors > during writes). That time goes up with a bigger cache and > with more fragmented write data in the cache requiring > more individual WRITE commands. RAID devices with gigabytes of cache are usually battery backed and lie about their cache type (they usually claim write through). This behaviour is exactly what we want because the flush is used to enforce write barriers for filesystem consistency, so we only want to flush volatile caches. The rare case you cite, where the RAID device is volatile and having difficulty flushing, we do want a failure because otherwise the filesystem will become unusable and the system will live lock ... barriers are sent down frequently under normal filesystem operation. The SCSI standards committees have been struggling with defining and detecting the difference between volatile and non-volatile cache and the heuristics we'd have to use to avoid annoying USB devices with detecting this don't look pretty. We always zero the SYNC_NV bit anyway, so even if the devices stopped lying, we'd be safe. > A better value would be the Recommended Command Timeout field > value reported in the REPORT SUPPORTED OPERATION CODES command, > if reported by the device server. That is supposed to account > for the worst case. > > For cases where that is not reported, exposing the multiplier > in sysfs would let the timeout for simple devices be set to > smaller values than complex devices. > > Also, in both sd_setup_flush_cmnd and sd_sync_cache: > cmd->cmnd[0] = SYNCHRONIZE_CACHE; > cmd->cmd_len = 10; > > SYNCHRONIZE CACHE (16) should be favored over SYNCHRONIZE > CACHE (10) unless SYNCHRONIZE CACHE (10) is not supported. For what reason. We usually go for the safe alternatives, which is 10 byte commands because they have the widest testing and greatest level of support. We don't do range flushes currently, so there doesn't seem to be a practical different. If we did support range flushes, we'd likely only use SC(16) on >2TB devices. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 17:26 +0200, Benoit Taine wrote: > We should prefer `const struct pci_device_id` over > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > This issue was reported by checkpatch. What kernel coding style? checkpatch isn't the arbiter of style, if that's the only problem. The DEFINE_PCI macro was a well reasoned addition when it was added in 2008. The problem was most people were getting the definition wrong. When we converted away from CONFIG_HOTPLUG, having this DEFINE_ meant that only one place needed changing instead of hundreds for PCI tables. The reason people were getting the PCI table wrong was mostly the init section specifiers which are now gone, but it has enough underlying utility (mostly constification) that I don't think we'd want to churn the kernel hugely to make a change to struct pci_table and then have to start detecting and fixing misuses. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: > On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: > > On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: > > > We should prefer `const struct pci_device_id` over > > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > > > This issue was reported by checkpatch. > > > > Honestly, I prefer the macro -- it stands-out more. Maybe the style > > guidelines and/or checkpatch should change instead? > > The macro is horrid, no other bus has this type of thing just to save a > few characters in typing OK, so this is the macro: #define DEFINE_PCI_DEVICE_TABLE(_table) \ const struct pci_device_id _table[] Could you explain what's so horrible? The reason it's useful today is that people forget the const (and sometimes the [] making it a true table instead of a pointer). If you use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it wrongly (good) and you automatically get the correct annotations. > , so why should PCI be "special" in this regard > anymore? I think the PCI usage dwarfs most other bus types now, so you could turn the question around. However, I don't think majority voting is a good guide to best practise; lets debate the merits for their own sake. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2014-07-18 at 16:44 +, KY Srinivasan wrote: > > > -Original Message- > > From: Christoph Hellwig (h...@infradead.org) [mailto:h...@infradead.org] > > Sent: Friday, July 18, 2014 8:11 AM > > To: KY Srinivasan > > Cc: Jens Axboe; James Bottomley; micha...@cs.wisc.edu; Christoph Hellwig > > (h...@infradead.org); linux-s...@vger.kernel.org; > > gre...@linuxfoundation.org; jasow...@redhat.com; linux- > > ker...@vger.kernel.org; oher...@suse.com; a...@canonical.com; > > de...@linuxdriverproject.org > > Subject: Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT > > from the basic I/O timeout > > > > On Thu, Jul 17, 2014 at 11:53:33PM +, KY Srinivasan wrote: > > > I still see this problem. There was talk of fixing it elsewhere. > > > > Well, what we have right not is entirely broken, given that the block layer > > doesn't initialize ->timeout on TYPE_FS requeuests. > > > > We either need to revert that initial commit or apply something like the > > attached patch as a quick fix. > I had sent this exact patch sometime back: > > http://www.spinics.net/lists/linux-scsi/msg75385.html Actually, no you didn't. The difference is in the derivation of the timeout. Christoph's patch is absolute in terms of SD_TIMEOUT; yours is relative to the queue timeout setting ... I thought there was a reason for preferring the relative version. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2014-07-18 at 10:00 -0700, Christoph Hellwig wrote: > On Wed, Jun 04, 2014 at 09:33:43AM -0700, K. Y. Srinivasan wrote: > > Commit ID: 7e660100d85af860e7ad763202fff717adcdaacd added code to derive the > > FLUSH_TIMEOUT from the basic I/O timeout. However, this patch did not use > > the > > basic I/O timeout of the device. Fix this bug. > > > > Signed-off-by: K. Y. Srinivasan > > Looks good to me, > > Reviewed-by: Christoph Hellwig > > (it will need some changes to apply to my tree, but I'm happy to do that > if I can get another review). > > James, does this look fine to you? Yes: Reviewed-by: James Bottomley James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/1] [SCSI] Fix a bug in deriving the FLUSH_TIMEOUT from the basic I/O timeout
On Fri, 2014-07-18 at 17:17 +, Elliott, Robert (Server Storage) wrote: > > > > From: James Bottomley [mailto:jbottom...@parallels.com] > > > > On Fri, 2014-07-18 at 00:51 +, Elliott, Robert (Server Storage) > > wrote: > ... > > > > > > Also, in both sd_setup_flush_cmnd and sd_sync_cache: > > > cmd->cmnd[0] = SYNCHRONIZE_CACHE; > > > cmd->cmd_len = 10; > > > > > > SYNCHRONIZE CACHE (16) should be favored over SYNCHRONIZE > > > CACHE (10) unless SYNCHRONIZE CACHE (10) is not supported. > > (sorry - meant "unless ... 16 is not supported") Yes, I guessed that? > > For what reason. We usually go for the safe alternatives, which is 10 > > byte commands because they have the widest testing and greatest level of > > support. We don't do range flushes currently, so there doesn't seem to > > be a practical different. If we did support range flushes, we'd likely > > only use SC(16) on >2TB devices. > > > > James > > A goal of the simplified SCSI feature set idea is to drop all the > short CDBs that have larger, more capable equivalents - don't carry > READ 6/10/12/16 and SYNCHRONIZE CACHE 10/16, just keep the 16-byte > versions. With modern serial IU-based protocols, short CDBs don't > save any transfer time. This will simplify design and testing on > both initiator and target sides. Competing command sets like NVMe > rightly point out that SCSI has too much legacy baggage - all you > need for IO is one READ, one WRITE, and one FLUSH command. But that's not relevant to us. This is the problem of practical vs standards approaches. We have to support older and buggy devices. Most small USB storage sticks die if they see 16 byte CDB commands because their interpreters. The more "legacy baggage" the standards committee dumps, the greater the number of heuristics OSs have to have to cope with the plethora of odd devices. > That's why SBC-3 ended up with these warning notes for all the > non-16 byte CDBs: > > NOTE 15 - Migration from the SYNCHRONIZE CACHE (10) command to > the SYNCHRONIZE CACHE (16) command is recommended for all > implementations. > > If the LBA field in SYNCHRONIZE CACHE went obsolete, then maybe > SYNCHRONIZE CACHE (10) would be kept instead of (16), but that > field is still present. So, (16) is the likely survivor. OK, but look at it from our point of view: The reply above justifies why we prefer 10 byte CDBs over 16. If the standards body ever did remove SC(10) completely, then we'd have to have yet another heuristic to recognise devices that don't support SC(10), but until that day, using SC(10) alone works in all cases, so is the better path for the OS. If you could, please get the standards body to recognise that the more command churn they introduce (in the name of rationalisation or whatever), the more problems they introduce for Operating Systems and the more likelihood (because of different people reading different revisions of standards) that we end up with compliance bugs in devices. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/25] Replace DEFINE_PCI_DEVICE_TABLE macro use
On Fri, 2014-07-18 at 11:17 -0700, Greg KH wrote: > On Fri, Jul 18, 2014 at 09:54:32AM -0700, James Bottomley wrote: > > On Fri, 2014-07-18 at 09:43 -0700, Greg KH wrote: > > > On Fri, Jul 18, 2014 at 12:22:13PM -0400, John W. Linville wrote: > > > > On Fri, Jul 18, 2014 at 05:26:47PM +0200, Benoit Taine wrote: > > > > > We should prefer `const struct pci_device_id` over > > > > > `DEFINE_PCI_DEVICE_TABLE` to meet kernel coding style guidelines. > > > > > This issue was reported by checkpatch. > > > > > > > > Honestly, I prefer the macro -- it stands-out more. Maybe the style > > > > guidelines and/or checkpatch should change instead? > > > > > > The macro is horrid, no other bus has this type of thing just to save a > > > few characters in typing > > > > OK, so this is the macro: > > > > #define DEFINE_PCI_DEVICE_TABLE(_table) \ > > const struct pci_device_id _table[] > > > > Could you explain what's so horrible? > > > > The reason it's useful today is that people forget the const (and > > sometimes the [] making it a true table instead of a pointer). If you > > use the DEFINE_PCI_DEVICE_TABLE macro, the compile breaks if you use it > > wrongly (good) and you automatically get the correct annotations. > > We have almost 1000 more uses of the non-macro version than the "macro" > version in the kernel today: > $ git grep -w DEFINE_PCI_DEVICE_TABLE | wc -l > 262 > $ git grep "const struct pci_device_id" | wc -l > 1254 > > My big complaint is that we need to be consistant, either pick one or > the other and stick to it. As the macro is the least used, it's easiest > to fix up, and it also is more consistant with all other kernel > subsystems which do not have such a macro. I've a weak preference for consistency, but not at the expense of hundreds of patches churning the kernel to remove an innocuous macro. Churn costs time and effort. > As there is no need for the __init macro mess anymore, there's no real > need for the DEFINE_PCI_DEVICE_TABLE macro either. I think checkpatch > will catch the use of non-const users for the id table already today, it > catches lots of other uses like this already. > > > > , so why should PCI be "special" in this regard > > > anymore? > > > > I think the PCI usage dwarfs most other bus types now, so you could turn > > the question around. However, I don't think majority voting is a good > > guide to best practise; lets debate the merits for their own sake. > > Not really "dwarf", USB is close with over 700 such structures: > $ git grep "const struct usb_device_id" | wc -l > 725 > > And i2c is almost just as big as PCI: > $ git grep "const struct i2c_device_id" | wc -l > 1223 > > So again, this macro is not consistent with the majority of PCI drivers, > nor with any other type of "device id" declaration in the kernel, which > is why I feel it should be removed. > > And finally, the PCI documentation itself says to not use this macro, so > this isn't a "new" thing. From Documentation/PCI/pci.txt: > > The ID table is an array of struct pci_device_id entries ending with an > all-zero entry. Definitions with static const are generally preferred. > Use of the deprecated macro DEFINE_PCI_DEVICE_TABLE should be avoided. > > That wording went into the file last December, when we last talked about > this and everyone in that discussion agreed to remove the macro for the > above reasons. > > Consistency matters. In this case, I don't think it does that much ... a cut and paste either way (from a macro or non-macro based driver) yields correct code. Since there's no bug here and no apparent way to misuse the macro, why bother? Anyway, it's PCI code ... let the PCI maintainer decide. However, if he does want this do it as one big bang patch via either the PCI or Trivial tree (latter because Jiří has experience doing this, but the former might be useful so the decider feels the pain ...) James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/3] [SCSI] storvsc: Add Hyper-V logical block provisioning tests
On Thu, 2014-07-24 at 08:56 +0100, Sitsofe Wheeler wrote: > Microsoft Hyper-V targets currently only claim SPC-2 compliance / no > compliance indicated even though they implement post SPC-2 features > which means those features are not tested for. Add a blacklist flag to > Hyper-V devices that forces said testing. This description is misleading: you don't force the test now, you force the driver to send unmap commands down to the device. > See https://lkml.org/lkml/2014/7/21/627 for the previous version of this > patch and https://lkml.org/lkml/2014/7/23/615 for example devices. > > Original-patch-by: K. Y. Srinivasan > Signed-off-by: Sitsofe Wheeler > --- > drivers/scsi/storvsc_drv.c | 10 +++--- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index 5ad2810..88b7173 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c > @@ -326,8 +326,6 @@ MODULE_PARM_DESC(storvsc_ringbuffer_size, "Ring buffer > size (bytes)"); > */ > static int storvsc_timeout = 180; > > -static int msft_blist_flags = BLIST_TRY_VPD_PAGES; > - > #define STORVSC_MAX_IO_REQUESTS 200 > > static void storvsc_on_channel_callback(void *context); > @@ -1444,12 +1442,10 @@ static int storvsc_device_configure(struct > scsi_device *sdevice) > sdevice->no_write_same = 1; > > /* > - * Add blist flags to permit the reading of the VPD pages even when > - * the target may claim SPC-2 compliance. MSFT targets currently > - * claim SPC-2 compliance while they implement post SPC-2 features. > - * With this patch we can correctly handle WRITE_SAME_16 issues. > + * Forcefully enable logical block provisioning testing. >*/ > - sdevice->sdev_bflags |= msft_blist_flags; > + sdevice->sdev_bflags |= BLIST_TRY_LBP; > + sdevice->try_lbp = 1; Really no way to this one. You're forcing unmap support on every hyper-v device; including spinning rust pass through ones which won't support it. The hyper-v storage interface has proven to be somewhat fragile, so it may explode when the device tries to send illegal command sense back. If you have a specific IDENTITY string for a faulty SSD that fails to report unmap support correctly, then we could quirk for that, but not everything attached to the hyper-v driver. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
On Fri, 2014-07-25 at 16:47 +, KY Srinivasan wrote: > > > -Original Message- > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com] > > Sent: Thursday, July 24, 2014 8:54 AM > > To: Sitsofe Wheeler > > Cc: Martin K. Petersen; Christoph Hellwig; KY Srinivasan; > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com; > > jasow...@redhat.com; jbottom...@parallels.com; linux- > > s...@vger.kernel.org > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests > > > > > "Sitsofe" == Sitsofe Wheeler writes: > > > > Sitsofe> So we can see it is really a SATA device that announces discard > > Sitsofe> correctly and supports discard through WRITE_SAME(16). > > > > No, that's the SATA device that announces support for DSM TRIM, and as a > > result the Linux SATL reports support for WRITE SAME(16) w. the UNMAP bit > > set and LBPME. > > > > Sitsofe> It is the act of passing it through Hyper-V that turned it into > > Sitsofe> a SCSI device that supports UNMAP (but not WRITE_SAME(16)), > > Sitsofe> doesn't announce its SCSI conformance number and doesn't > > Sitsofe> correctly announce which features it supports. Surely in this > > Sitsofe> case it's reasonable to quirk our way around the problem? > > > > No. That's an issue in Hyper-V that'll you'll have to take up with > > Microsoft. I > > don't know what their passthrough limitations are for SCSI-ATA translation. > > Maybe K. Y. has some insight into this? > > For the pass through case, the host validates the request and passes > the request to the device. > However, not all scsi commands are passed through even though the > device it is being passed through > may support the command. WRITE_SAME is one such command. Consequently, > in the EVPD page, > we will set state indicating that WRITE_SAME is not supported (even if > the device supports it). I think you haven't appreciated the problem: He's passing a SATA SSD via the SCSI hyper-v interface. That means that the windows host is doing SCSI<->ATA translation. The problem is that the Windows translation layer (SATL) looks to be incomplete and it's not correctly translating the IDENTIFY bit that corresponds to TRIM to the correct VPD pages so consequently, Linux won't send UNMAP commands to the device (to be translated back to TRIM). We already know this is a bug in the Windows SATL which needs fixing (if you could report it and get a fix, that would be great) and that we're not going to be able to work around this automatically in Linux because the proposed patch would have us unconditionally try UNMAP for all Hyper-V devices. The current proposed fix is to enable UNMAP manually via sysfs in the guest boot scripts, but obviously that means that Hyper-V guests with direct pass through of SSDs need operator intervention to turn on TRIM. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests
On Mon, 2014-07-28 at 19:05 +, KY Srinivasan wrote: > > > -Original Message- > > From: Martin K. Petersen [mailto:martin.peter...@oracle.com] > > Sent: Monday, July 28, 2014 12:03 PM > > To: KY Srinivasan > > Cc: Martin K. Petersen; Sitsofe Wheeler; Christoph Hellwig; > > gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; oher...@suse.com; a...@canonical.com; > > jasow...@redhat.com; jbottom...@parallels.com; linux- > > s...@vger.kernel.org > > Subject: Re: [PATCH v2 3/3] [SCSI] Make LBP quirk skip lbpme checks tests > > > > > "KY" == KY Srinivasan writes: > > > > KY, > > > > KY> "At the time thin-provisioning was defined, the discovery > > KY> information was first proposed in READ CAPACITY 16 command. And > > then > > KY> moved into the new dedicated VPD page - B2h. You can see the > > KY> information reported in this VPD page is richer than READ CAPACITY > > KY> 16 command. As this transition happened during we added the feature, > > KY> Windows uses the newer method that based on VPD page B2h. It looks > > KY> Linux tries to use both new and old method which is weird to me." > > > > The READ CAPACITY(16) response is not optional. > > Ok; that settles the issue then. I will attempt to get it fixed on Windows. Like Martin says, this isn't optional either/or; it's mandatory to support the RC 16 bits. If you don't want to get into playing the messenger between us and the windows guys on SCSI standards, we'd be happy to communicate directly, either by email or a phone meeting. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > On 8/17/20 12:48 PM, Kees Cook wrote: > > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > > > On 8/17/20 12:29 PM, Kees Cook wrote: > > > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > > > > > On 8/17/20 2:15 AM, Allen Pais wrote: > > > > > > From: Allen Pais > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > and from_tasklet() to pass the tasklet pointer explicitly. > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' that > > > > > is just container_of? container_of in the code would be > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > from_tasklet is doing. > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > As I mentioned in the other thread, I think this makes things > > > > much more readable. It's the same thing that the timer_struct > > > > conversion did (added a container_of wrapper) to avoid the > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > But then it should use a generic name, instead of each sub-system > > > using some random name that makes people look up exactly what it > > > does. I'm not huge fan of the container_of() redundancy, but > > > adding private variants of this doesn't seem like the best way > > > forward. Let's have a generic helper that does this, and use it > > > everywhere. > > > > I'm open to suggestions, but as things stand, these kinds of > > treewide > > On naming? Implementation is just as it stands, from_tasklet() is > totally generic which is why I objected to it. from_member()? Not > great with naming... But I can see this going further and then we'll > suddenly have tons of these. It's not good for readability. Since both threads seem to have petered out, let me suggest in kernel.h: #define cast_out(ptr, container, member) \ container_of(ptr, typeof(*container), member) It does what you want, the argument order is the same as container_of with the only difference being you name the containing structure instead of having to specify its type. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On Tue, 2020-08-18 at 13:10 -0700, Kees Cook wrote: > On Tue, Aug 18, 2020 at 01:00:33PM -0700, James Bottomley wrote: > > On Mon, 2020-08-17 at 13:02 -0700, Jens Axboe wrote: > > > On 8/17/20 12:48 PM, Kees Cook wrote: > > > > On Mon, Aug 17, 2020 at 12:44:34PM -0700, Jens Axboe wrote: > > > > > On 8/17/20 12:29 PM, Kees Cook wrote: > > > > > > On Mon, Aug 17, 2020 at 06:56:47AM -0700, Jens Axboe wrote: > > > > > > > On 8/17/20 2:15 AM, Allen Pais wrote: > > > > > > > > From: Allen Pais > > > > > > > > > > > > > > > > In preparation for unconditionally passing the > > > > > > > > struct tasklet_struct pointer to all tasklet > > > > > > > > callbacks, switch to using the new tasklet_setup() > > > > > > > > and from_tasklet() to pass the tasklet pointer > > > > > > > > explicitly. > > > > > > > > > > > > > > Who came up with the idea to add a macro 'from_tasklet' > > > > > > > that > > > > > > > is just container_of? container_of in the code would be > > > > > > > _much_ more readable, and not leave anyone guessing wtf > > > > > > > from_tasklet is doing. > > > > > > > > > > > > > > I'd fix that up now before everything else goes in... > > > > > > > > > > > > As I mentioned in the other thread, I think this makes > > > > > > things > > > > > > much more readable. It's the same thing that the > > > > > > timer_struct > > > > > > conversion did (added a container_of wrapper) to avoid the > > > > > > ever-repeating use of typeof(), long lines, etc. > > > > > > > > > > But then it should use a generic name, instead of each sub- > > > > > system > > > > > using some random name that makes people look up exactly what > > > > > it > > > > > does. I'm not huge fan of the container_of() redundancy, but > > > > > adding private variants of this doesn't seem like the best > > > > > way > > > > > forward. Let's have a generic helper that does this, and use > > > > > it > > > > > everywhere. > > > > > > > > I'm open to suggestions, but as things stand, these kinds of > > > > treewide > > > > > > On naming? Implementation is just as it stands, from_tasklet() is > > > totally generic which is why I objected to it. from_member()? Not > > > great with naming... But I can see this going further and then > > > we'll > > > suddenly have tons of these. It's not good for readability. > > > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as > > container_of with the only difference being you name the containing > > structure instead of having to specify its type. > > I like this! Shall I send this to Linus to see if this can land in > -rc2 for use going forward? Sure ... he's probably been lurking on this thread anyway ... it's about time he got off his arse^Wthe fence and made an executive decision ... James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On Wed, 2020-08-19 at 07:00 -0600, Jens Axboe wrote: > On 8/18/20 1:00 PM, James Bottomley wrote: [...] > > Since both threads seem to have petered out, let me suggest in > > kernel.h: > > > > #define cast_out(ptr, container, member) \ > > container_of(ptr, typeof(*container), member) > > > > It does what you want, the argument order is the same as > > container_of with the only difference being you name the containing > > structure instead of having to specify its type. > > Not to incessantly bike shed on the naming, but I don't like > cast_out, it's not very descriptive. And it has connotations of > getting rid of something, which isn't really true. Um, I thought it was exactly descriptive: you're casting to the outer container. I thought about following the C++ dynamic casting style, so out_cast(), but that seemed a bit pejorative. What about outer_cast()? > FWIW, I like the from_ part of the original naming, as it has some > clues as to what is being done here. Why not just from_container()? > That should immediately tell people what it does without having to > look up the implementation, even before this becomes a part of the > accepted coding norm. I'm not opposed to container_from() but it seems a little less descriptive than outer_cast() but I don't really care. I always have to look up container_of() when I'm using it so this would just be another macro of that type ... James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] block: convert tasklets to use new tasklet_setup() API
On Wed, 2020-08-19 at 21:54 +0530, Allen wrote: > > [...] > > > > Since both threads seem to have petered out, let me suggest in > > > > kernel.h: > > > > > > > > #define cast_out(ptr, container, member) \ > > > > container_of(ptr, typeof(*container), member) > > > > > > > > It does what you want, the argument order is the same as > > > > container_of with the only difference being you name the > > > > containing structure instead of having to specify its type. > > > > > > Not to incessantly bike shed on the naming, but I don't like > > > cast_out, it's not very descriptive. And it has connotations of > > > getting rid of something, which isn't really true. > > > > Um, I thought it was exactly descriptive: you're casting to the > > outer container. I thought about following the C++ dynamic casting > > style, so out_cast(), but that seemed a bit pejorative. What about > > outer_cast()? > > > > > FWIW, I like the from_ part of the original naming, as it has > > > some clues as to what is being done here. Why not just > > > from_container()? That should immediately tell people what it > > > does without having to look up the implementation, even before > > > this becomes a part of the accepted coding norm. > > > > I'm not opposed to container_from() but it seems a little less > > descriptive than outer_cast() but I don't really care. I always > > have to look up container_of() when I'm using it so this would just > > be another macro of that type ... > > > > So far we have a few which have been suggested as replacement > for from_tasklet() > > - out_cast() or outer_cast() > - from_member(). > - container_from() or from_container() > > from_container() sounds fine, would trimming it a bit work? like > from_cont(). I'm fine with container_from(). It's the same form as container_of() and I think we need urgent agreement to not stall everything else so the most innocuous name is likely to get the widest acceptance. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 3/5] Drivers: scsi: storvsc: Implement multi-channel support
On Tue, 2013-06-04 at 12:05 -0700, K. Y. Srinivasan wrote: > Implement multi-channel support for the storage devices. This doesn't compile: CC [M] drivers/scsi/storvsc_drv.o drivers/scsi/storvsc_drv.c: In function ‘handle_sc_creation’: drivers/scsi/storvsc_drv.c:763:35: error: ‘struct vmbus_channel’ has no member named ‘primary_channel’ drivers/scsi/storvsc_drv.c: In function ‘handle_multichannel_storage’: drivers/scsi/storvsc_drv.c:805:2: error: implicit declaration of function ‘vmbus_set_sc_create_callback’ [-Werror=implicit-function-declaration] drivers/scsi/storvsc_drv.c:812:2: error: implicit declaration of function ‘vmbus_are_subchannels_present’ [-Werror=implicit-function-declaration] drivers/scsi/storvsc_drv.c: In function ‘storvsc_on_channel_callback’: drivers/scsi/storvsc_drv.c:1223:13: error: ‘struct vmbus_channel’ has no member named ‘primary_channel’ drivers/scsi/storvsc_drv.c:1224:19: error: ‘struct vmbus_channel’ has no member named ‘primary_channel’ drivers/scsi/storvsc_drv.c: In function ‘storvsc_do_io’: drivers/scsi/storvsc_drv.c:1341:2: error: implicit declaration of function ‘vmbus_get_outgoing_channel’ [-Werror=implicit-function-declaration] drivers/scsi/storvsc_drv.c:1341:19: warning: assignment makes pointer from integer without a cast [enabled by default] cc1: some warnings being treated as errors make[2]: *** [drivers/scsi/storvsc_drv.o] Error 1 I assume this is a cross tree dependency? What's the relevant branch I need? James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V3 0/5] Drivers: scsi: storvsc
On Wed, 2013-06-26 at 12:58 +, KY Srinivasan wrote: > > > -Original Message- > > From: KY Srinivasan > > Sent: Monday, June 17, 2013 9:32 AM > > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; > > h...@infradead.org; linux-s...@vger.kernel.org > > Subject: RE: [PATCH V3 0/5] Drivers: scsi: storvsc > > > > > > > > > -Original Message- > > > From: KY Srinivasan > > > Sent: Tuesday, June 11, 2013 9:02 AM > > > To: KY Srinivasan; gre...@linuxfoundation.org; > > > linux-ker...@vger.kernel.org; > > > de...@linuxdriverproject.org; oher...@suse.com; jbottom...@parallels.com; > > > h...@infradead.org; linux-s...@vger.kernel.org > > > Subject: RE: [PATCH V3 0/5] Drivers: scsi: storvsc > > > > > > > > > > > > > -Original Message- > > > > From: K. Y. Srinivasan [mailto:k...@microsoft.com] > > > > Sent: Tuesday, June 04, 2013 3:05 PM > > > > To: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > > > > de...@linuxdriverproject.org; oher...@suse.com; > > jbottom...@parallels.com; > > > > h...@infradead.org; linux-s...@vger.kernel.org > > > > Cc: KY Srinivasan > > > > Subject: [PATCH V3 0/5] Drivers: scsi: storvsc > > > > > > > > This set adds multi-channel support as well synthetic Fibre Channel > > > > support > > > > to storvsc. The multi-channel support depends on infrastructure in the > > VMBUS > > > > driver. Greg has already checked in the relevant patches to VMBUS. > > > > > > > > I had posted an earlier version of this patch-set that included the > > > > VMBUS > > > > related changes. I have since separated the VMBUS chages and these have > > > > already been > > > > checked in. > > > > > > > > In this version, based on comments from James, the timeout is no longer > > > > a > > > > module > > > > parameter. > > > > > > James, > > > > > > I think I have addressed all the comments that you had; if not, please > > > let me > > > know. > > > > Ping. > > James, > > Let me know if I should re-send the patches. Just FYI, my workflow involves using imap flags to tag stuff for actions and then using a set of date sorted evolution search folders to translate the flags into work queues. The search folders are by flag and thread, so a side effect of the way evolution does date ordering is that it's ordered by the most recent last, so the date it takes for thread ordering is the most recent email in the thread. Originally, I thought of this as an annyoing evolution bug because I wanted the folder ordered by date of the patch submission. However, later I came to appreciate that it meant currently actively discussed patches moved down in my workqueue automatically, so now I quite like it as a feature. It does mean, however, that the net effect of sending very frequent pings about a patch set is to cause that patch set to move down in my work queue and, unfortunately, even coming from a domain that habitually breaks threading doesn't save you because evolution helpfully has a set of heuristic rules to rethread outlook breakage. James ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 0/4] Hyper-V TRIM support
On Fri, 2013-09-13 at 13:58 +0100, Andy Whitcroft wrote: > tl;dr -- enable TRIM support for Hyper-V emulated disks. > > The Hyper-V hypervisor can support TRIM for its devices, advertising this > via the appropriate VPD pages. However the emulated disks only claim > to be SPC-2 devices. According to the specs VPD pages (in general) did > exist at SPC-2 but the specific pages we interogate for the TRIM support > did not until SPC-3 therefore the kernel avoids reading the relevant pages > for SPC-2 devices and prevents TRIM from being offered for these devices. > Additionally at SPC-2 we prefer ReadCapacity10 over ReadCapacity16 and > unless we use RC16 we will not identify the device as TRIM capable also > preventing TRIM being offered. > > As the VPD page zero does list which pages are valid for each device, it > could be argued that we could simply attempt to use these pages for all > devices which claim to be SPC-2 and above. While this seems valid the > code documents a number of devices which take badly to having even VPD > page 0 interogated even when supposedly supported. Therefore it seems > appropriate to add a scsi device flag to allow a device to request SPC-3 > VPD pages be used when only at SPC-2. > > Similarly for the ReadCapacity selection it seems dangerous to invert > the order for all SPC-2 devices. So it seems appropriate to add a scsi > device flag to request we try RC16 before RC10 (mirroring the existing > flag for the opposite). > > The following four patches add the two scsi device flags and select those > flags for the Hyper-V emulated disks. > > Patches against v3.11. > > Comments? This is an awful lot of contortions (which don't seem to have any other users on the horizon) to support a device that's not standards compliant. What about this, it's simple, it does the right thing and it's contained in the driver of the problem device. James --- diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index 83ec1aa..bd86a4b 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -1438,6 +1438,12 @@ static int storvsc_device_configure(struct scsi_device *sdevice) sdevice->no_write_same = 1; + /* +* hyper-v is stupid and lies about its capabilities +* If we pretend to be SPC-3, we send RC16 which activates trim +*/ + sdevice->scsi_level = SCSI_SPC_3; + return 0; } ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
